[PATCH 0/2] TCP conntrack fixes in TCP option handling

[PATCH 0/2] TCP conntrack fixes in TCP option handling

[Jozsef Kadlecsik]

Hi Patrick,

Michael M. Builov reported several problems with the TCP option handling
code in the TCP connection tracking code. The next patches, based on his
suggestions, solve all the issues. Please apply them as bugfixes.

Best regards,
Jozsef

Jozsef Kadlecsik (2):
  Incorrect handling of invalid TCP option in TCP connection tracking
  Wrong multiplication of TCPOLEN_TSTAMP_ALIGNED in tcp_sack skips
    fastpath

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

[PATCH 1/2] Incorrect handling of invalid TCP option in TCP connection tracking

[Jozsef Kadlecsik]

Michael M. Builov reported that in the tcp_options and tcp_sack functions
of netfilter TCP conntrack the incorrect handling of invalid TCP option
with too big opsize may lead to read access beyond tcp-packet or buffer
allocated on stack (netfilter bugzilla #738). The fix is to stop parsing
the options at detecting the broken option.

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/nf_conntrack_proto_tcp.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index 37bf943..afc4ab7 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -409,7 +409,7 @@ static void tcp_options(const struct sk_buff *skb,
 			if (opsize < 2) /* "silly options" */
 				return;
 			if (opsize > length)
-				break;	/* don't parse partial options */
+				return;	/* don't parse partial options */
 
 			if (opcode == TCPOPT_SACK_PERM
 			    && opsize == TCPOLEN_SACK_PERM)
@@ -469,7 +469,7 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 			if (opsize < 2) /* "silly options" */
 				return;
 			if (opsize > length)
-				break;	/* don't parse partial options */
+				return;	/* don't parse partial options */
 
 			if (opcode == TCPOPT_SACK
 			    && opsize >= (TCPOLEN_SACK_BASE
-- 
1.7.0.4

[PATCH 2/2] Wrong multiplication of TCPOLEN_TSTAMP_ALIGNED in tcp_sack skips fastpath

[Jozsef Kadlecsik]

The wrong multiplication of TCPOLEN_TSTAMP_ALIGNED by 4 skips the fast path
for the timestamp-only option. Bug reported by Michael M. Builov (netfilter
bugzilla #738).

Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>
---
 net/netfilter/nf_conntrack_proto_tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/nf_conntrack_proto_tcp.c b/net/netfilter/nf_conntrack_proto_tcp.c
index afc4ab7..8235b86 100644
--- a/net/netfilter/nf_conntrack_proto_tcp.c
+++ b/net/netfilter/nf_conntrack_proto_tcp.c
@@ -447,7 +447,7 @@ static void tcp_sack(const struct sk_buff *skb, unsigned int dataoff,
 	BUG_ON(ptr == NULL);
 
 	/* Fast path for timestamp-only option */
-	if (length == TCPOLEN_TSTAMP_ALIGNED*4
+	if (length == TCPOLEN_TSTAMP_ALIGNED
 	    && *(__be32 *)ptr == htonl((TCPOPT_NOP << 24)
 				       | (TCPOPT_NOP << 16)
 				       | (TCPOPT_TIMESTAMP << 8)
-- 
1.7.0.4

Re: [PATCH 2/2] Wrong multiplication of TCPOLEN_TSTAMP_ALIGNED in tcp_sack skips fastpath

[Patrick McHardy]

On 30.08.2011 15:35, Jozsef Kadlecsik wrote:
> The wrong multiplication of TCPOLEN_TSTAMP_ALIGNED by 4 skips the fast path
> for the timestamp-only option. Bug reported by Michael M. Builov (netfilter
> bugzilla #738).
> 
> Signed-off-by: Jozsef Kadlecsik <kadlec@blackhole.kfki.hu>

Also applied, thanks.

Re: [PATCH 1/2] Incorrect handling of invalid TCP option in TCP connection tracking

[Patrick McHardy]

On 30.08.2011 15:35, Jozsef Kadlecsik wrote:
> Michael M. Builov reported that in the tcp_options and tcp_sack functions
> of netfilter TCP conntrack the incorrect handling of invalid TCP option
> with too big opsize may lead to read access beyond tcp-packet or buffer
> allocated on stack (netfilter bugzilla #738). The fix is to stop parsing
> the options at detecting the broken option.

Applied, thanks Jozsef.