Patchwork net: fix infinite loop in __skb_recv_datagram()

login
register
mail settings
Submitter Eric Dumazet
Date Feb. 12, 2013, 4:16 p.m.
Message ID <1360685813.13993.12.camel@edumazet-glaptop>
Download mbox | patch
Permalink /patch/219877/
State Accepted
Delegated to: David Miller
Headers show

Comments

Eric Dumazet - Feb. 12, 2013, 4:16 p.m.
From: Eric Dumazet <edumazet@google.com>

Tommi was fuzzing with trinity and reported the following problem :

commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
missed that a raw socket receive queue can contain skbs with no payload.

We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
wait_for_packet() is not prepared to skip these skbs.

[   83.541011] INFO: rcu_sched detected stalls on CPUs/tasks: {}
(detected by 0, t=26002 jiffies, g=27673, c=27672, q=75)
[   83.541011] INFO: Stall ended before state dump start
[  108.067010] BUG: soft lockup - CPU#0 stuck for 22s! [trinity-child31:2847]
...
[  108.067010] Call Trace:
[  108.067010]  [<ffffffff818cc103>] __skb_recv_datagram+0x1a3/0x3b0
[  108.067010]  [<ffffffff818cc33d>] skb_recv_datagram+0x2d/0x30
[  108.067010]  [<ffffffff819ed43d>] rawv6_recvmsg+0xad/0x240
[  108.067010]  [<ffffffff818c4b04>] sock_common_recvmsg+0x34/0x50
[  108.067010]  [<ffffffff818bc8ec>] sock_recvmsg+0xbc/0xf0
[  108.067010]  [<ffffffff818bf31e>] sys_recvfrom+0xde/0x150
[  108.067010]  [<ffffffff81ca4329>] system_call_fastpath+0x16/0x1b

Reported-by: Tommi Rantala <tt.rantala@gmail.com>
Tested-by: Tommi Rantala <tt.rantala@gmail.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Pavel Emelyanov <xemul@parallels.com>
---
 net/core/datagram.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)



--
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
Pavel Emelyanov - Feb. 12, 2013, 4:18 p.m.
On 02/12/2013 08:16 PM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> Tommi was fuzzing with trinity and reported the following problem :
> 
> commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
> missed that a raw socket receive queue can contain skbs with no payload.
> 
> We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
> wait_for_packet() is not prepared to skip these skbs.
> 
> [   83.541011] INFO: rcu_sched detected stalls on CPUs/tasks: {}
> (detected by 0, t=26002 jiffies, g=27673, c=27672, q=75)
> [   83.541011] INFO: Stall ended before state dump start
> [  108.067010] BUG: soft lockup - CPU#0 stuck for 22s! [trinity-child31:2847]
> ...
> [  108.067010] Call Trace:
> [  108.067010]  [<ffffffff818cc103>] __skb_recv_datagram+0x1a3/0x3b0
> [  108.067010]  [<ffffffff818cc33d>] skb_recv_datagram+0x2d/0x30
> [  108.067010]  [<ffffffff819ed43d>] rawv6_recvmsg+0xad/0x240
> [  108.067010]  [<ffffffff818c4b04>] sock_common_recvmsg+0x34/0x50
> [  108.067010]  [<ffffffff818bc8ec>] sock_recvmsg+0xbc/0xf0
> [  108.067010]  [<ffffffff818bf31e>] sys_recvfrom+0xde/0x150
> [  108.067010]  [<ffffffff81ca4329>] system_call_fastpath+0x16/0x1b
> 
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Tested-by: Tommi Rantala <tt.rantala@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Pavel Emelyanov <xemul@parallels.com>

Acked-by: Pavel Emelyanov <xemul@parallels.com>

Thanks!

> ---
>  net/core/datagram.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index 0337e2b..368f9c3 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -187,7 +187,7 @@ struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
>  		skb_queue_walk(queue, skb) {
>  			*peeked = skb->peeked;
>  			if (flags & MSG_PEEK) {
> -				if (*off >= skb->len) {
> +				if (*off >= skb->len && skb->len) {
>  					*off -= skb->len;
>  					continue;
>  				}
> 
> 
> .
> 


--
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 - Feb. 12, 2013, 9:07 p.m.
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 12 Feb 2013 08:16:53 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> Tommi was fuzzing with trinity and reported the following problem :
> 
> commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
> missed that a raw socket receive queue can contain skbs with no payload.
> 
> We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
> wait_for_packet() is not prepared to skip these skbs.
 ...
> Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> Tested-by: Tommi Rantala <tt.rantala@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, 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
Hannes Frederic Sowa - Feb. 15, 2013, 12:41 p.m.
On Tue, Feb 12, 2013 at 04:07:33PM -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 12 Feb 2013 08:16:53 -0800
> 
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > Tommi was fuzzing with trinity and reported the following problem :
> > 
> > commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
> > missed that a raw socket receive queue can contain skbs with no payload.
> > 
> > We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
> > wait_for_packet() is not prepared to skip these skbs.
>  ...
> > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> > Tested-by: Tommi Rantala <tt.rantala@gmail.com>
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> Applied, thanks.

This issue got a CVE: http://seclists.org/oss-sec/2013/q1/310
Perhaps it's something that should go to stable?

Thanks,

  Hannes

--
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
Ben Hutchings - Feb. 15, 2013, 5:43 p.m.
On Fri, 2013-02-15 at 13:41 +0100, Hannes Frederic Sowa wrote:
> On Tue, Feb 12, 2013 at 04:07:33PM -0500, David Miller wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 12 Feb 2013 08:16:53 -0800
> > 
> > > From: Eric Dumazet <edumazet@google.com>
> > > 
> > > Tommi was fuzzing with trinity and reported the following problem :
> > > 
> > > commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
> > > missed that a raw socket receive queue can contain skbs with no payload.
> > > 
> > > We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
> > > wait_for_packet() is not prepared to skip these skbs.
> >  ...
> > > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
> > > Tested-by: Tommi Rantala <tt.rantala@gmail.com>
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > 
> > Applied, thanks.
> 
> This issue got a CVE: http://seclists.org/oss-sec/2013/q1/310
> Perhaps it's something that should go to stable?

David has already worked that out for himself:
http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Ben.
Hannes Frederic Sowa - Feb. 15, 2013, 5:55 p.m.
On Fri, Feb 15, 2013 at 05:43:30PM +0000, Ben Hutchings wrote:
> David has already worked that out for himself:
> http://patchwork.ozlabs.org/bundle/davem/stable/?state=*

Thanks, didn't know where to look.

--
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 - Feb. 15, 2013, 6:56 p.m.
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 15 Feb 2013 13:41:41 +0100

> On Tue, Feb 12, 2013 at 04:07:33PM -0500, David Miller wrote:
>> From: Eric Dumazet <eric.dumazet@gmail.com>
>> Date: Tue, 12 Feb 2013 08:16:53 -0800
>> 
>> > From: Eric Dumazet <edumazet@google.com>
>> > 
>> > Tommi was fuzzing with trinity and reported the following problem :
>> > 
>> > commit 3f518bf745 (datagram: Add offset argument to __skb_recv_datagram)
>> > missed that a raw socket receive queue can contain skbs with no payload.
>> > 
>> > We can loop in __skb_recv_datagram() with MSG_PEEK mode, because
>> > wait_for_packet() is not prepared to skip these skbs.
>>  ...
>> > Reported-by: Tommi Rantala <tt.rantala@gmail.com>
>> > Tested-by: Tommi Rantala <tt.rantala@gmail.com>
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>> 
>> Applied, thanks.
> 
> This issue got a CVE: http://seclists.org/oss-sec/2013/q1/310
> Perhaps it's something that should go to stable?

It's already queued up in my -stable queue.


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

Patch

diff --git a/net/core/datagram.c b/net/core/datagram.c
index 0337e2b..368f9c3 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -187,7 +187,7 @@  struct sk_buff *__skb_recv_datagram(struct sock *sk, unsigned int flags,
 		skb_queue_walk(queue, skb) {
 			*peeked = skb->peeked;
 			if (flags & MSG_PEEK) {
-				if (*off >= skb->len) {
+				if (*off >= skb->len && skb->len) {
 					*off -= skb->len;
 					continue;
 				}