diff mbox

possible kernel oops from user MSS

Message ID 20101110.124119.102563803.davem@davemloft.net
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

David Miller Nov. 10, 2010, 8:41 p.m. UTC 
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(-)

Comments

Shan Wei Nov. 11, 2010, 5:15 a.m. UTC | #1 
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.
David Miller Nov. 11, 2010, 5:33 a.m. UTC | #2 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Shan Wei Nov. 11, 2010, 5:37 a.m. UTC | #3 
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.
Min Zhang Nov. 12, 2010, 10:59 p.m. UTC | #4 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 12, 2010, 11:26 p.m. UTC | #5 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Li Yewang Nov. 23, 2010, 2:48 a.m. UTC | #6 
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?



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 23, 2010, 2:59 a.m. UTC | #7 
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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;
 		}