On Fri, 4 Nov 2022 19:43:33 +1100 David Gibson <david(a)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(a)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