Re: [PATCH 10/10] tcp: Fix small error in tcp_seq_init() time handling

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2466 bytes --]

On Mon, Nov 07, 2022 at 07:08:46PM +0100, Stefano Brivio wrote:
> On Fri,  4 Nov 2022 19:43:33 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > It looks like tcp_seq_init() is supposed to advance the sequence number
> > by one every 32ns.  However we only right shift the ns part of the timespec
> > not the seconds part, meaning that we'll advance by an extra 32 steps on
> > each second.
> > 
> > I don't know if that's exploitable in any way, but it doesn't appear to be
> > the intent, nor what RFC 6528 suggests.
> 
> Oh, oops, nice catch.
> 
> Well, as long as the step, modulo 32 bits, is not 0, it's still
> arguably the 250 KHz / 4 µs period clock from RFC 793, so there's no

Well, except for the fact it's a 31.24 MHz / 32 ns clock.  I assumed
there was a good reason for that.

> practical difference (other than the overflow period). See also the
> note in RFC 1948:
> 
>    More precisely, RFC 793 specifies that the 32-bit counter be
>    incremented by 1 in the low-order position about every 4
>    microseconds.  Instead, Berkeley-derived kernels increment it by a
>    constant every second [...]
> 
> I kind of wonder if adding a time non-linearity like the unintended one
> I added actually increases entropy.

Right, I don't know.

> But indeed ~4 seconds overflow is also not intended, and we should just
> stick to RFC 6528.

Well... 4s overflow, yes, but not I think a 4s period before
repeating.  Again, I don't know enough about this stuff to analyze
whether that's important or not.

> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tcp.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tcp.c b/tcp.c
> > index 59e03ff..941fafb 100644
> > --- a/tcp.c
> > +++ b/tcp.c
> > @@ -2027,8 +2027,8 @@ static void tcp_seq_init(const struct ctx *c, struct tcp_conn *conn,
> >  
> >  	seq = siphash_36b((uint8_t *)&in, c->tcp.hash_secret);
> >  
> > -	ns = now->tv_sec * 1E9;
> > -	ns += now->tv_nsec >> 5; /* 32ns ticks, overflows 32 bits every 137s */
> > +	/* 32ns ticks, overflows 32 bits every 137s */
> > +	ns = (now->tv_sec * 1E9 + now->tv_nsec) >> 5;
> >  
> >  	conn->seq_to_tap = seq + ns;
> >  }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]