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

[Stefano Brivio <sbrivio@redhat.com>]

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
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.

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

> 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;
>  }

-- 
Stefano