Patchwork [net,2/2] net: dev_queue_xmit_nit: fix potential NULL ptr dereference

login
register
mail settings
Submitter Daniel Borkmann
Date Jan. 8, 2013, 6:51 p.m.
Message ID <1357671093-9605-3-git-send-email-dborkman@redhat.com>
Download mbox | patch
Permalink /patch/210475/
State Rejected
Delegated to: David Miller
Headers show

Comments

Daniel Borkmann - Jan. 8, 2013, 6:51 p.m.
Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
skb->users instead of skb_clone()'') introduced a skb_clone in
dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
be injected into pt_prev->func().

Cc: Changli Gao <xiaosuo@gmail.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Eric Dumazet - Jan. 8, 2013, 7:22 p.m.
On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
> Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
> skb->users instead of skb_clone()'') introduced a skb_clone in
> dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
> be injected into pt_prev->func().
> 
> Cc: Changli Gao <xiaosuo@gmail.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
>  net/core/dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 723dcd0..6c35c33 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1827,7 +1827,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>  			pt_prev = ptype;
>  		}
>  	}
> -	if (pt_prev)
> +	if (skb2 && pt_prev)
>  		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>  	rcu_read_unlock();
>  }

My opinion is this patch is not needed.

pt_prev can be set only if skb2 is not NULL



--
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
Daniel Borkmann - Jan. 8, 2013, 7:38 p.m.
On 01/08/2013 08:22 PM, Eric Dumazet wrote:
> On Tue, 2013-01-08 at 19:51 +0100, Daniel Borkmann wrote:
>> Commit 71d9dec24dce548bf699815c976cf063ad9257e2 (``net: increase
>> skb->users instead of skb_clone()'') introduced a skb_clone in
>> dev_queue_xmit_nit that, when NULL, leaves the loop, but can still
>> be injected into pt_prev->func().
>>
>> Cc: Changli Gao <xiaosuo@gmail.com>
>> Cc: Eric Dumazet <eric.dumazet@gmail.com>
>> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
>> ---
>>   net/core/dev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 723dcd0..6c35c33 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -1827,7 +1827,7 @@ static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
>>   			pt_prev = ptype;
>>   		}
>>   	}
>> -	if (pt_prev)
>> +	if (skb2 && pt_prev)
>>   		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
>>   	rcu_read_unlock();
>>   }
>
> My opinion is this patch is not needed.
>
> pt_prev can be set only if skb2 is not NULL

Agreed, I missed that, 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

Patch

diff --git a/net/core/dev.c b/net/core/dev.c
index 723dcd0..6c35c33 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1827,7 +1827,7 @@  static void dev_queue_xmit_nit(struct sk_buff *skb, struct net_device *dev)
 			pt_prev = ptype;
 		}
 	}
-	if (pt_prev)
+	if (skb2 && pt_prev)
 		pt_prev->func(skb2, skb->dev, pt_prev, skb->dev);
 	rcu_read_unlock();
 }