[PATCH] packet: fix potential use after free

[PATCH] packet: fix potential use after free

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

As soon skb is queued into sk_receive_queue, it can be consumed,
so its racy to access skb->len.

Given that sk_data_ready() / sock_def_readable() don't really care, just
use 0 instead of skb->len

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Xi Wang <xii@google.com>
---
 net/packet/af_packet.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 72e0c71fb01d..373d557e22ee 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -1848,7 +1848,8 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
 	skb->dropcount = atomic_read(&sk->sk_drops);
 	__skb_queue_tail(&sk->sk_receive_queue, skb);
 	spin_unlock(&sk->sk_receive_queue.lock);
-	sk->sk_data_ready(sk, skb->len);
+	/* Remember: we can not reference skb after this point */
+	sk->sk_data_ready(sk, 0);
 	return 0;
 
 drop_n_acct:

Re: [PATCH] packet: fix potential use after free

[Daniel Borkmann]

On 04/10/2014 03:22 AM, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> As soon skb is queued into sk_receive_queue, it can be consumed,
> so its racy to access skb->len.
>
> Given that sk_data_ready() / sock_def_readable() don't really care, just
> use 0 instead of skb->len
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Xi Wang <xii@google.com>

Acked-by: Daniel Borkmann <dborkman@redhat.com>

Seems to be there since pre 2005 ...

Re: [PATCH] packet: fix potential use after free

[Eric Dumazet]

On Thu, 2014-04-10 at 10:19 +0200, Daniel Borkmann wrote:
> On 04/10/2014 03:22 AM, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > As soon skb is queued into sk_receive_queue, it can be consumed,
> > so its racy to access skb->len.
> >
> > Given that sk_data_ready() / sock_def_readable() don't really care, just
> > use 0 instead of skb->len
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Cc: Xi Wang <xii@google.com>
> 
> Acked-by: Daniel Borkmann <dborkman@redhat.com>
> 
> Seems to be there since pre 2005 ...

Yeah, and many other places have the same error, a full audit is
probably needed.

Re: [PATCH] packet: fix potential use after free

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 10 Apr 2014 08:06:58 -0700

> On Thu, 2014-04-10 at 10:19 +0200, Daniel Borkmann wrote:
>> On 04/10/2014 03:22 AM, Eric Dumazet wrote:
>> > From: Eric Dumazet <edumazet@google.com>
>> >
>> > As soon skb is queued into sk_receive_queue, it can be consumed,
>> > so its racy to access skb->len.
>> >
>> > Given that sk_data_ready() / sock_def_readable() don't really care, just
>> > use 0 instead of skb->len
>> >
>> > Signed-off-by: Eric Dumazet <edumazet@google.com>
>> > Cc: Xi Wang <xii@google.com>
>> 
>> Acked-by: Daniel Borkmann <dborkman@redhat.com>
>> 
>> Seems to be there since pre 2005 ...
> 
> Yeah, and many other places have the same error, a full audit is
> probably needed.

Not only sock_def_readable(), but all implementations of this callback absolutely
do not care about the length argument.

I'm working on a patch to kill the argument completely.

Re: [PATCH] packet: fix potential use after free

[Daniel Borkmann]

On 04/11/2014 09:59 PM, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 10 Apr 2014 08:06:58 -0700
>
>> On Thu, 2014-04-10 at 10:19 +0200, Daniel Borkmann wrote:
>>> On 04/10/2014 03:22 AM, Eric Dumazet wrote:
>>>> From: Eric Dumazet <edumazet@google.com>
>>>>
>>>> As soon skb is queued into sk_receive_queue, it can be consumed,
>>>> so its racy to access skb->len.
>>>>
>>>> Given that sk_data_ready() / sock_def_readable() don't really care, just
>>>> use 0 instead of skb->len
>>>>
>>>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>>>> Cc: Xi Wang <xii@google.com>
>>>
>>> Acked-by: Daniel Borkmann <dborkman@redhat.com>
>>>
>>> Seems to be there since pre 2005 ...
>>
>> Yeah, and many other places have the same error, a full audit is
>> probably needed.
>
> Not only sock_def_readable(), but all implementations of this callback absolutely
> do not care about the length argument.
>
> I'm working on a patch to kill the argument completely.

Sounds good! Seen you've already pushed that, thanks!