: netfilter: nf_conntrack_tcp: fixing to check the lower bound of valid ACK

netfilter: nf_conntrack_tcp: fixing to check the lower bound of valid ACK

Upstream commit 84ebe1c:

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.

---
commit 9524a965e043b5aac8770b39a0d444da30655ec4
tree 6f88f35682e10fad204228331ec89b50c6477577
parent 44e450bf173eee791911a56f7e65a30d94608cea
author Jozsef Kadlecsik <[email protected]> Mon, 07 Jul 2008 15:52:48 +0200 committer Patrick McHardy <[email protected]> Mon, 07 Jul 2008 15:52:48 +0200

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

index 6256795..fc43e22 100644
@@ -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).
 		 */

This is a note to let you know that we have just queued up the patch titled

Subject: netfilter: nf_conntrack_tcp: fixing to check the lower bound of valid ACK

to the 2.6.25-stable tree. Its filename is

netfilter-nf_conntrack_tcp-fixing-to-check-the-lower-bound-of-valid-ack.patch

A git repo of this tree can be found at
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

From [email protected] Wed Jul 16 22:28:33 2008
From: Jozsef Kadlecsik <[email protected]>
Date: Mon, 07 Jul 2008 15:57:03 +0200
Subject: netfilter: nf_conntrack_tcp: fixing to check the lower bound of valid ACK
To: [email protected]
Cc: Netfilter Development Mailinglist <[email protected]>, "David S. Miller" <[email protected]>
Message-ID: <[email protected]>

From: Jozsef Kadlecsik <[email protected]>

Upstream commit 84ebe1c:

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.

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

@@ -332,12 +332,13 @@ static unsigned int get_conntrack_index(
 
    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
 		 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).
 		 */

Patches currently in stable-queue which might be from [email protected] are

queue-2.6.25/netfilter-nf_conntrack_tcp-fixing-to-check-the-lower-bound-of-valid-ack.patch --
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html