possible kernel oops from user MSS

possible kernel oops from user MSS

[Steve Chen]

Hello

With commit f5fff5dc8a7a3f395b0525c02ba92c95d42b7390, a user program
can pass in TCP_MAXSEG of 12 (or TCPOLEN_TSTAMP_ALIGNED), and cause
kernel oops with division by 0
 in tcp_select_initial_window.  One way to prevent it is to change the
minimum value for TCP_MAXSEG in do_tcp_setsockopt from 8 to some value
over 12.  Two questions.

1.  Is this the right solution?
2.  If it is, what is a good minimum value?

Thanks,

Steve

Re: possible kernel oops from user MSS

[David Miller]

From: Steve Chen <schen@mvista.com>
Date: Wed, 10 Nov 2010 07:24:51 -0600

> With commit f5fff5dc8a7a3f395b0525c02ba92c95d42b7390, a user program
> can pass in TCP_MAXSEG of 12 (or TCPOLEN_TSTAMP_ALIGNED), and cause
> kernel oops with division by 0
>  in tcp_select_initial_window.  One way to prevent it is to change the
> minimum value for TCP_MAXSEG in do_tcp_setsockopt from 8 to some value
> over 12.  Two questions.
> 
> 1.  Is this the right solution?
> 2.  If it is, what is a good minimum value?

Thanks Steve, I'll fix this like so:

--------------------
tcp: Increase TCP_MAXSEG socket option minimum.

As noted by Steve Chen, since commit
f5fff5dc8a7a3f395b0525c02ba92c95d42b7390 ("tcp: advertise MSS
requested by user") we can end up with a situation where
tcp_select_initial_window() does a divide by a zero (or
even negative) mss value.

The problem is that sometimes we subtract TCPOLEN_TSTAMP_ALIGNED
from the mss.

Fix this by increasing the minimum from 8 to 8 plus the value
of TCPOLEN_TSTATMP_ALIGNED.

Reported-by: Steve Chen <schen@mvista.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 245603c..6b0eb4d 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2246,7 +2246,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 		/* Values greater than interface MTU won't take effect. However
 		 * at the point when this call is done we typically don't yet
 		 * know which interface is going to be used */
-		if (val < 8 || val > MAX_TCP_WINDOW) {
+		if (val < TCPOLEN_TSTAMP_ALIGNED + 8 || val > MAX_TCP_WINDOW) {
 			err = -EINVAL;
 			break;
 		}
-- 
1.7.3.2

Re: possible kernel oops from user MSS

[Shan Wei]

David Miller wrote, at 11/11/2010 04:41 AM:
> From: Steve Chen <schen@mvista.com>
> Date: Wed, 10 Nov 2010 07:24:51 -0600
> 
>> With commit f5fff5dc8a7a3f395b0525c02ba92c95d42b7390, a user program
>> can pass in TCP_MAXSEG of 12 (or TCPOLEN_TSTAMP_ALIGNED), and cause
>> kernel oops with division by 0
>>  in tcp_select_initial_window.  One way to prevent it is to change the
>> minimum value for TCP_MAXSEG in do_tcp_setsockopt from 8 to some value
>> over 12.  Two questions.
>>
>> 1.  Is this the right solution?
>> 2.  If it is, what is a good minimum value?
> 
> Thanks Steve, I'll fix this like so:
> 
> --------------------
> tcp: Increase TCP_MAXSEG socket option minimum.
> 
> As noted by Steve Chen, since commit
> f5fff5dc8a7a3f395b0525c02ba92c95d42b7390 ("tcp: advertise MSS
> requested by user") we can end up with a situation where
> tcp_select_initial_window() does a divide by a zero (or
> even negative) mss value.
> 
> The problem is that sometimes we subtract TCPOLEN_TSTAMP_ALIGNED
> from the mss.
> 
> Fix this by increasing the minimum from 8 to 8 plus the value
> of TCPOLEN_TSTATMP_ALIGNED.

In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes)
and TCPOLEN_MD5SIG_ALIGNED(20 bytes).

This fix is still not perfect.

The minimum value of TCP_MAXSEG is 20 tytes, tcp_select_initial_window() still be 
called with negative mss value.

-- 
Best Regards
-----
Shan Wei


> Reported-by: Steve Chen <schen@mvista.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  net/ipv4/tcp.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 245603c..6b0eb4d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -2246,7 +2246,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
>  		/* Values greater than interface MTU won't take effect. However
>  		 * at the point when this call is done we typically don't yet
>  		 * know which interface is going to be used */
> -		if (val < 8 || val > MAX_TCP_WINDOW) {
> +		if (val < TCPOLEN_TSTAMP_ALIGNED + 8 || val > MAX_TCP_WINDOW) {
>  			err = -EINVAL;
>  			break;
>  		}

Re: possible kernel oops from user MSS

[David Miller]

From: Shan Wei <shanwei@cn.fujitsu.com>
Date: Thu, 11 Nov 2010 13:15:01 +0800

> In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes)
> and TCPOLEN_MD5SIG_ALIGNED(20 bytes).

If you knew this, why didn't you mention it in your initial report? :-/

I'll make the minimum 64 or something like that.

Re: possible kernel oops from user MSS

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Wed, 10 Nov 2010 21:33:13 -0800 (PST)

> I'll make the minimum 64 or something like that.

Here is the patch I will use:

--------------------
tcp: Increase TCP_MAXSEG socket option minimum.

As noted by Steve Chen, since commit
f5fff5dc8a7a3f395b0525c02ba92c95d42b7390 ("tcp: advertise MSS
requested by user") we can end up with a situation where
tcp_select_initial_window() does a divide by a zero (or
even negative) mss value.

The problem is that sometimes we effectively subtract
TCPOLEN_TSTAMP_ALIGNED and/or TCPOLEN_MD5SIG_ALIGNED from the mss.

Fix this by increasing the minimum from 8 to 64.

Reported-by: Steve Chen <schen@mvista.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 245603c..0814199 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2246,7 +2246,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 		/* Values greater than interface MTU won't take effect. However
 		 * at the point when this call is done we typically don't yet
 		 * know which interface is going to be used */
-		if (val < 8 || val > MAX_TCP_WINDOW) {
+		if (val < 64 || val > MAX_TCP_WINDOW) {
 			err = -EINVAL;
 			break;
 		}
-- 
1.7.3.2

Re: possible kernel oops from user MSS

[Shan Wei]

David Miller wrote, at 11/11/2010 01:33 PM:
> From: Shan Wei <shanwei@cn.fujitsu.com>
> Date: Thu, 11 Nov 2010 13:15:01 +0800
> 
>> In tcp_connect_init(), if tcp_header_len includes TCPOLEN_TSTAMP_ALIGNED(12 bytes)
>> and TCPOLEN_MD5SIG_ALIGNED(20 bytes).
> 
> If you knew this, why didn't you mention it in your initial report? :-/

Firstly reported by Steve Chen, not me. :-)
I just review your patch.
 
> I'll make the minimum 64 or something like that.

Welcome. 


-- 
Best Regards
-----
Shan Wei

Re: possible kernel oops from user MSS

[Min Zhang]

Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp:
Increase TCP_MAXSEG socket option minimum"). What is the reason
TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is
TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8?

Or is it better to use TCP_MIN_MSS from tcp.h:

/* Minimal accepted MSS. It is (60+60+8) - (20+20). */
#define TCP_MIN_MSS        88U

Re: possible kernel oops from user MSS

[David Miller]

From: Min Zhang <mzhang@mvista.com>
Date: Fri, 12 Nov 2010 14:59:58 -0800

> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp:
> Increase TCP_MAXSEG socket option minimum"). What is the reason
> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is
> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8?
> 
> Or is it better to use TCP_MIN_MSS from tcp.h:
> 
> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */
> #define TCP_MIN_MSS        88U

I suppose TCP_MIN_MSS would be better to use, I'll make that
change, thanks.

Re: possible kernel oops from user MSS

[Li Yewang]

At 2010-11-13 7:26, David Miller wrote:
> From: Min Zhang<mzhang@mvista.com>
> Date: Fri, 12 Nov 2010 14:59:58 -0800
>
>> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp:
>> Increase TCP_MAXSEG socket option minimum"). What is the reason
>> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is
>> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8?
>>
>> Or is it better to use TCP_MIN_MSS from tcp.h:
>>
>> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */
>> #define TCP_MIN_MSS        88U
>
> I suppose TCP_MIN_MSS would be better to use, I'll make that
> change, thanks.

   David, do you have plan to fix this bug using TCP_MIN_MSS?

Re: possible kernel oops from user MSS

[David Miller]

From: Li Yewang <lyw@cn.fujitsu.com>
Date: Tue, 23 Nov 2010 10:48:46 +0800

> 
> 
> At 2010-11-13 7:26, David Miller wrote:
>> From: Min Zhang<mzhang@mvista.com>
>> Date: Fri, 12 Nov 2010 14:59:58 -0800
>>
>>> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp:
>>> Increase TCP_MAXSEG socket option minimum"). What is the reason
>>> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is
>>> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8?
>>>
>>> Or is it better to use TCP_MIN_MSS from tcp.h:
>>>
>>> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */
>>> #define TCP_MIN_MSS        88U
>>
>> I suppose TCP_MIN_MSS would be better to use, I'll make that
>> change, thanks.
> 
>   David, do you have plan to fix this bug using TCP_MIN_MSS?

I will, it's deep in my backlog and pretty low priority right now.

Re: possible kernel oops from user MSS

[David Miller]

From: Min Zhang <mzhang@mvista.com>
Date: Fri, 12 Nov 2010 14:59:58 -0800

> Regarding commit 7a1abd08d52fdeddb3e9a5a33f2f15cc6a5674d2 ("tcp:
> Increase TCP_MAXSEG socket option minimum"). What is the reason
> TCP_MAXSEG minimum be 64? Isn't the exact be 40 which is
> TCPOLEN_MD5SIG_ALIGNED(20) + TCPOLEN_TSTAMP_ALIGNED(12) + 8?
> 
> Or is it better to use TCP_MIN_MSS from tcp.h:
> 
> /* Minimal accepted MSS. It is (60+60+8) - (20+20). */
> #define TCP_MIN_MSS        88U

Committed to net-2.6:

--------------------
>From c39508d6f118308355468314ff414644115a07f3 Mon Sep 17 00:00:00 2001
From: David S. Miller <davem@davemloft.net>
Date: Wed, 24 Nov 2010 11:47:22 -0800
Subject: [PATCH] tcp: Make TCP_MAXSEG minimum more correct.

Use TCP_MIN_MSS instead of constant 64.

Reported-by: Min Zhang <mzhang@mvista.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/ipv4/tcp.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 0814199..f15c36a 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2246,7 +2246,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
 		/* Values greater than interface MTU won't take effect. However
 		 * at the point when this call is done we typically don't yet
 		 * know which interface is going to be used */
-		if (val < 64 || val > MAX_TCP_WINDOW) {
+		if (val < TCP_MIN_MSS || val > MAX_TCP_WINDOW) {
 			err = -EINVAL;
 			break;
 		}
-- 
1.7.3.2