[PATCH] Fixing to check the lower bound of valid ACK

[PATCH] Fixing to check the lower bound of valid ACK

[Jozsef Kadlecsik]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3238 bytes --]

Hi Patrick,

Lost connections was reported by Thomas Bätzler (running 2.6.25 kernel) on 
the netfilter mailing list (see the thread "Weird nat/conntrack Problem 
with PASV FTP upload"). He provided tcpdump recordings which helped to 
find a long lingering bug in conntrack.

In TCP connection tracking, checking the lower bound of valid ACK could 
lead to mark valid packets as INVALID because:

 - We have got a "higher or equal" inequality, but the test checked
   the "higher" condition only; fixed.
 - If the packet contains a SACK option, it could occur that the ACK
   value was before the left edge of our (S)ACK "window": if a previous
   packet from the other party intersected the right edge of the window
   of the receiver, we could move forward the window parameters beyond
   accepting a valid ack. Therefore in this patch we check the rightmost
   SACK edge instead of the ACK value in the lower bound of valid (S)ACK
   test.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

---
 net/netfilter/nf_conntrack_proto_tcp.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 6256795..fc43e22 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -332,12 +332,13 @@ static unsigned int get_conntrack_index(const struct tcphdr *tcph)
 
    I.   Upper bound for valid data:	seq <= sender.td_maxend
    II.  Lower bound for valid data:	seq + len >= sender.td_end - receiver.td_maxwin
-   III.	Upper bound for valid ack:      sack <= receiver.td_end
-   IV.	Lower bound for valid ack:	ack >= receiver.td_end - MAXACKWINDOW
+   III.	Upper bound for valid (s)ack:   sack <= receiver.td_end
+   IV.	Lower bound for valid (s)ack:	sack >= receiver.td_end - MAXACKWINDOW
 
-   where sack is the highest right edge of sack block found in the packet.
+   where sack is the highest right edge of sack block found in the packet
+   or ack in the case of packet without SACK option.
 
-   The upper bound limit for a valid ack is not ignored -
+   The upper bound limit for a valid (s)ack is not ignored -
    we doesn't have to deal with fragments.
 */
 
@@ -607,12 +608,12 @@ static int tcp_in_window(const struct nf_conn *ct,
 		 before(seq, sender->td_maxend + 1),
 		 after(end, sender->td_end - receiver->td_maxwin - 1),
 		 before(sack, receiver->td_end + 1),
-		 after(ack, receiver->td_end - MAXACKWINDOW(sender)));
+		 after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1));
 
 	if (before(seq, sender->td_maxend + 1) &&
 	    after(end, sender->td_end - receiver->td_maxwin - 1) &&
 	    before(sack, receiver->td_end + 1) &&
-	    after(ack, receiver->td_end - MAXACKWINDOW(sender))) {
+	    after(sack, receiver->td_end - MAXACKWINDOW(sender) - 1)) {
 		/*
 		 * Take into account window scaling (RFC 1323).
 		 */
-- 
1.5.3.4

Best regards,
Jozsef
-
E-mail  : kadlec@blackhole.kfki.hu, kadlec@mail.kfki.hu
PGP key : http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address : KFKI Research Institute for Particle and Nuclear Physics
          H-1525 Budapest 114, POB. 49, Hungary

Re: [PATCH] Fixing to check the lower bound of valid ACK

[Patrick McHardy]

Jozsef Kadlecsik wrote:
> Hi Patrick,
> 
> Lost connections was reported by Thomas Bätzler (running 2.6.25 kernel) on 
> the netfilter mailing list (see the thread "Weird nat/conntrack Problem 
> with PASV FTP upload"). He provided tcpdump recordings which helped to 
> find a long lingering bug in conntrack.
> 
> In TCP connection tracking, checking the lower bound of valid ACK could 
> lead to mark valid packets as INVALID because:
> 
>  - We have got a "higher or equal" inequality, but the test checked
>    the "higher" condition only; fixed.
>  - If the packet contains a SACK option, it could occur that the ACK
>    value was before the left edge of our (S)ACK "window": if a previous
>    packet from the other party intersected the right edge of the window
>    of the receiver, we could move forward the window parameters beyond
>    accepting a valid ack. Therefore in this patch we check the rightmost
>    SACK edge instead of the ACK value in the lower bound of valid (S)ACK
>    test.

Applied, thanks. I'll also push this patch to -stable once
its in Linus' tree.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html