PATCH: uninitialized memory access in tcp_parse_options

PATCH: uninitialized memory access in tcp_parse_options

[Mathieu Lacage]

[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]

valgrind reports the following error:

==15996== Conditional jump or move depends on uninitialised value(s)
==15996==    at 0x6E63E4C: tcp_parse_options (tcp_input.c:3776)
==15996==    by 0x6E856A3: tcp_check_req (tcp_minisocks.c:532)
==15996==    by 0x6E7F0C6: tcp_v4_hnd_req (tcp_ipv4.c:1492)
==15996==    by 0x6E7F55A: tcp_v4_do_rcv (tcp_ipv4.c:1571)
==15996==    by 0x6E808C5: tcp_v4_rcv (tcp_ipv4.c:1690)
==15996==    by 0x6E2DA7B: ip_local_deliver_finish (ip_input.c:231)
==15996==    by 0x6E2DE0C: ip_local_deliver (netfilter.h:206)
==15996==    by 0x6E2E940: ip_rcv_finish (dst.h:255)
==15996==    by 0x6E2F17C: ip_rcv (netfilter.h:206)
==15996==    by 0x6D53D0E: __netif_receive_skb (dev.c:2873)
==15996==    by 0x6D5521F: process_backlog (dev.c:3305)
==15996==    by 0x6D55A20: net_rx_action (dev.c:3435)

The attached patch (generated against net-next-2.6) fixes that error by
making sure that user_mss is correctly initialized at the start of
tcp_parse_options, just like saw_tstamp is initialized at the start of
this function. To try to be coherent, this patch also removes the
redundant initialization of saw_tstamp from the caller, tcp_check_req.

hope this helps,
Mathieu
-- 
Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
Tel: +33 4 9238 5056

[-- Attachment #2: tcp-options.patch --]
[-- Type: text/x-patch, Size: 855 bytes --]

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index ae3ec15..8b713ab 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3751,6 +3751,7 @@ void tcp_parse_options(struct sk_buff *skb, struct tcp_options_received *opt_rx,
 
 	ptr = (unsigned char *)(th + 1);
 	opt_rx->saw_tstamp = 0;
+	opt_rx->user_mss = 0;
 
 	while (length > 0) {
 		int opcode = *ptr++;
diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 794c2e1..21e47e7 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -527,7 +527,6 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 	__be32 flg = tcp_flag_word(th) & (TCP_FLAG_RST|TCP_FLAG_SYN|TCP_FLAG_ACK);
 	int paws_reject = 0;
 
-	tmp_opt.saw_tstamp = 0;
 	if (th->doff > (sizeof(struct tcphdr)>>2)) {
 		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 

Re: PATCH: uninitialized memory access in tcp_parse_options

[Mitchell Erblich]

Mathieu Lacage,

		The standard default for TCP with IPv4 is 536, which
		translates to 576 MTU.

		Thus, why don't you init mss to 536?

		Mitchell Erblich
		===============

On Jun 21, 2010, at 6:34 AM, Mathieu Lacage wrote:

> valgrind reports the following error:
> 
> ==15996== Conditional jump or move depends on uninitialised value(s)
> ==15996==    at 0x6E63E4C: tcp_parse_options (tcp_input.c:3776)
> ==15996==    by 0x6E856A3: tcp_check_req (tcp_minisocks.c:532)
> ==15996==    by 0x6E7F0C6: tcp_v4_hnd_req (tcp_ipv4.c:1492)
> ==15996==    by 0x6E7F55A: tcp_v4_do_rcv (tcp_ipv4.c:1571)
> ==15996==    by 0x6E808C5: tcp_v4_rcv (tcp_ipv4.c:1690)
> ==15996==    by 0x6E2DA7B: ip_local_deliver_finish (ip_input.c:231)
> ==15996==    by 0x6E2DE0C: ip_local_deliver (netfilter.h:206)
> ==15996==    by 0x6E2E940: ip_rcv_finish (dst.h:255)
> ==15996==    by 0x6E2F17C: ip_rcv (netfilter.h:206)
> ==15996==    by 0x6D53D0E: __netif_receive_skb (dev.c:2873)
> ==15996==    by 0x6D5521F: process_backlog (dev.c:3305)
> ==15996==    by 0x6D55A20: net_rx_action (dev.c:3435)
> 
> The attached patch (generated against net-next-2.6) fixes that error by
> making sure that user_mss is correctly initialized at the start of
> tcp_parse_options, just like saw_tstamp is initialized at the start of
> this function. To try to be coherent, this patch also removes the
> redundant initialization of saw_tstamp from the caller, tcp_check_req.
> 
> hope this helps,
> Mathieu
> -- 
> Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
> Tel: +33 4 9238 5056
> <tcp-options.patch>

Re: PATCH: uninitialized memory access in tcp_parse_options

[Mathieu Lacage]

On Mon, 2010-06-21 at 11:02 -0700, Mitchell Erblich wrote:

> 		The standard default for TCP with IPv4 is 536, which
> 		translates to 576 MTU.
> 
> 		Thus, why don't you init mss to 536?

I don't know: I am not a tcp expert and I am not sure I really
understand the way this function is expected to be used by callers but I
sent a patch to make sure that someone would feel compelled to find the
right fix. It looks like callers of this function are expected to
initialize the fields themselves so, the idea of doing the
initialization in tcp_parse_options is probably bad.

Mathieu
-- 
Mathieu Lacage <mathieu.lacage@sophia.inria.fr>
Tel: +33 4 9238 5056

Re: PATCH: uninitialized memory access in tcp_parse_options

[Eric Dumazet]

Le lundi 21 juin 2010 à 15:34 +0200, Mathieu Lacage a écrit :
> valgrind reports the following error:
> 
> ==15996== Conditional jump or move depends on uninitialised value(s)
> ==15996==    at 0x6E63E4C: tcp_parse_options (tcp_input.c:3776)
> ==15996==    by 0x6E856A3: tcp_check_req (tcp_minisocks.c:532)
> ==15996==    by 0x6E7F0C6: tcp_v4_hnd_req (tcp_ipv4.c:1492)
> ==15996==    by 0x6E7F55A: tcp_v4_do_rcv (tcp_ipv4.c:1571)
> ==15996==    by 0x6E808C5: tcp_v4_rcv (tcp_ipv4.c:1690)
> ==15996==    by 0x6E2DA7B: ip_local_deliver_finish (ip_input.c:231)
> ==15996==    by 0x6E2DE0C: ip_local_deliver (netfilter.h:206)
> ==15996==    by 0x6E2E940: ip_rcv_finish (dst.h:255)
> ==15996==    by 0x6E2F17C: ip_rcv (netfilter.h:206)
> ==15996==    by 0x6D53D0E: __netif_receive_skb (dev.c:2873)
> ==15996==    by 0x6D5521F: process_backlog (dev.c:3305)
> ==15996==    by 0x6D55A20: net_rx_action (dev.c:3435)
> 
> The attached patch (generated against net-next-2.6) fixes that error by
> making sure that user_mss is correctly initialized at the start of
> tcp_parse_options, just like saw_tstamp is initialized at the start of
> this function. To try to be coherent, this patch also removes the
> redundant initialization of saw_tstamp from the caller, tcp_check_req.
> 
> hope this helps,
> Mathieu


Mathieu, this valgrind splat is a false positive, and your fix is not
necessary or at the right place.

In tcp_check_req(), we call tcp_parse_options() only to get the
saw_tstamp indication. So only initialize this field to 0 before calling
tcp_parse_options()

If you want to avoid valgrind false positive at this point, without
introducing bug for other tcp_parse_options() callers, a better fix
would be following patch.

Thanks

diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
index 794c2e1..4e758ac 100644
--- a/net/ipv4/tcp_minisocks.c
+++ b/net/ipv4/tcp_minisocks.c
@@ -520,14 +520,13 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
 			   struct request_sock *req,
 			   struct request_sock **prev)
 {
-	struct tcp_options_received tmp_opt;
+	struct tcp_options_received tmp_opt = {0};
 	u8 *hash_location;
 	struct sock *child;
 	const struct tcphdr *th = tcp_hdr(skb);
 	__be32 flg = tcp_flag_word(th) & (TCP_FLAG_RST|TCP_FLAG_SYN|TCP_FLAG_ACK);
 	int paws_reject = 0;
 
-	tmp_opt.saw_tstamp = 0;
 	if (th->doff > (sizeof(struct tcphdr)>>2)) {
 		tcp_parse_options(skb, &tmp_opt, &hash_location, 0);
 

Re: PATCH: uninitialized memory access in tcp_parse_options

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 26 Jun 2010 07:58:04 +0200

> If you want to avoid valgrind false positive at this point, without
> introducing bug for other tcp_parse_options() callers, a better fix
> would be following patch.
> 
> Thanks
> 
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index 794c2e1..4e758ac 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -520,14 +520,13 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>  			   struct request_sock *req,
>  			   struct request_sock **prev)
>  {
> -	struct tcp_options_received tmp_opt;
> +	struct tcp_options_received tmp_opt = {0};
>  	u8 *hash_location;
>  	struct sock *child;

That's a 28 byte memset() in the connect fast-path.  We shouldn't eat this
just to placate a valgrind miscue. :-)