linux-kernel - Re: [PATCH 4/5] NET: Make ts_recent

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Wed, 24 Oct 2007 14:08:36 -0400
From:	Chuck Lever <chuck.lever@...cle.com>
To:	David Miller <davem@...emloft.net>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] NET: Make ts_recent_stamp unsigned

David Miller wrote:
> From: Chuck Lever <chuck.lever@...cle.com>
> Date: Tue, 23 Oct 2007 11:44:28 -0400
> 
>> The get_seconds() function returns an unsigned long.  To prevent incorrect
>> comparison results between values saved in ts_recent_stamp and later
>> invocations of get_seconds(), change the type of ts_recent_stamp to match
>> the return type of get_seconds().
>>
>> Signed-off-by: Chuck Lever <chuck.lever@...cle.com>
>> Cc: <netdev@...r.kernel.org>
> 
> I see two potential problems with this patch:
> 
> 1) If you update struct tcp_options_received you should also
>    update struct tcp_timewait_sock similarly.
> 
>    The fact that you missed this suggests that you didn't
>    grep the tree to see how else this variable is used and
>    this makes me extra concerned about this patch's correctness.

Perhaps the result of wishful thinking on my part.  I was hoping for a 
small and self-contained change.

> 2) There are computations in the TCP stack using this member that
>    probably care about the signedness, such as:
> 
> net/ipv4/tcp_ipv4.c:			     get_seconds() - tcptw->tw_ts_recent_stamp > 1))) {
> include/net/tcp.h:	if (get_seconds() >= rx_opt->ts_recent_stamp + TCP_PAWS_24DAYS)
> include/net/tcp.h:	if (get_seconds() >= rx_opt->ts_recent_stamp + TCP_PAWS_24DAYS)
> 
>    We should make sure we understand what is expected here, and
>    why it would still be correct after making both ts_recent_stamp
>    members unsigned.

Agreed.

I wonder how one could construct a series of mixed case time stamp 
comparisons *on purpose* (and without documentation of this assumption) 
that produces consistently correct results.

 From the invocations of get_seconds() that I sampled, the design of 
these comparisons seems to assume that both sides of the comparison are 
non-negative.  However, they do not seem to account for time crossing zero.

View attachment "chuck.lever.vcf" of type "text/x-vcard" (316 bytes)