netdev - Re: [PATCH] [TCP]: Fix logic breakage due to DSACK separation

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] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 16 Jun 2007 02:04:25 +0300 (EEST)
From:	"Ilpo Järvinen" <ilpo.jarvinen@...sinki.fi>
To:	David Miller <davem@...emloft.net>
cc:	Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH] [TCP]: Fix logic breakage due to DSACK separation

On Fri, 15 Jun 2007, David Miller wrote:

> From: "Ilpo_Järvinen" <ilpo.jarvinen@...sinki.fi>
> Date: Fri, 15 Jun 2007 16:07:37 +0300 (EEST)
> 
> > Commit 6f74651ae626ec672028587bc700538076dfbefb is found guilty
> > of breaking DSACK counting, which should be done only for the
> > SACK block reported by the DSACK instead of every SACK block
> > that is received along with DSACK information.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
> 
> Thanks for performing such rigorous audits and finding
> regressions like this!

They just come across, one thing leads to another, which again leads to 
another and so on... ...I was just trying to address the concerns you 
noted about tcp-2.6 patchset a while ago and came across other issues...

There are still some things I must think carefully in sacktag processing 
since it does not validate start_seq and end_seq at all which can be 
abused currently at least in tcp-2.6. ...I would rather put end to the 
whole russian roulette in tcp-2.6 sacktag rather than fix/think individual 
cases and leave future modifications of it similarily hazardous. It's not 
very clear to me how to handle all possible cases of invalid SACK blocks 
though, perhaps TCP should just ignore such sack blocks without doing 
any processing based on them, i.e., ignore them whenever start_seq-end_seq 
does not fully fit to snd_una-snd_nxt (expect DSACK of course, which 
should be processed if it's between undo_marker-snd_nxt). Do you have any 
thoughts about this?

It seems to me that net-2.6 is safe in this respect (probably just by 
accident) but the analysis wasn't that trivial, my main concern was 
tcp_fragment's pkt_len argument, if it ever becomes > skb->len, a boom 
would result (and perhaps zero has similar issues)! I couldn't find 
breakage analytically (but I could be wrong in it due to mind-boggling 
number of negations :-)) nor by bruteforcing seqno combinations near 0, 
skb->len and 2^31 wrap.

> Patch applied.

...I think it should go to stable as well.

-- 
 i.