[PATCH] 8021q: fix a potential memory leak

[PATCH] 8021q: fix a potential memory leak

[roy.qing.li]

From: Li RongQing <roy.qing.li@gmail.com>

skb_cow called in vlan_reorder_header does not free the skb when it failed,
and vlan_reorder_header returns NULL to reset original skb when it is called
in vlan_untag, lead to a memory leak.

Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
---
 net/8021q/vlan_core.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
index 9012b1c..75d4277 100644
--- a/net/8021q/vlan_core.c
+++ b/net/8021q/vlan_core.c
@@ -114,8 +114,11 @@ EXPORT_SYMBOL(vlan_dev_vlan_proto);
 
 static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
 {
-	if (skb_cow(skb, skb_headroom(skb)) < 0)
+	if (skb_cow(skb, skb_headroom(skb)) < 0) {
+		kfree_skb(skb);
 		return NULL;
+	}
+
 	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 2 * ETH_ALEN);
 	skb->mac_header += VLAN_HLEN;
 	return skb;
-- 
1.7.10.4

Re: [PATCH] 8021q: fix a potential memory leak

[Eric Dumazet]

On Wed, 2014-06-18 at 13:46 +0800, roy.qing.li@gmail.com wrote:
> From: Li RongQing <roy.qing.li@gmail.com>
> 
> skb_cow called in vlan_reorder_header does not free the skb when it failed,
> and vlan_reorder_header returns NULL to reset original skb when it is called
> in vlan_untag, lead to a memory leak.
> 
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>
> ---
>  net/8021q/vlan_core.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index 9012b1c..75d4277 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -114,8 +114,11 @@ EXPORT_SYMBOL(vlan_dev_vlan_proto);
>  
>  static struct sk_buff *vlan_reorder_header(struct sk_buff *skb)
>  {
> -	if (skb_cow(skb, skb_headroom(skb)) < 0)
> +	if (skb_cow(skb, skb_headroom(skb)) < 0) {
> +		kfree_skb(skb);
>  		return NULL;
> +	}
> +
>  	memmove(skb->data - ETH_HLEN, skb->data - VLAN_ETH_HLEN, 2 * ETH_ALEN);
>  	skb->mac_header += VLAN_HLEN;
>  	return skb;

Good catch.

Acked-by: Eric Dumazet <edumazet@google.com>

Note that the 'goto err_free;' in caller is misleading as well, as skb
is NULL.

vlan_reorder_header() could return an error code instead of a skb...

Re: [PATCH] 8021q: fix a potential memory leak

[David Miller]

From: roy.qing.li@gmail.com
Date: Wed, 18 Jun 2014 13:46:02 +0800

> From: Li RongQing <roy.qing.li@gmail.com>
> 
> skb_cow called in vlan_reorder_header does not free the skb when it failed,
> and vlan_reorder_header returns NULL to reset original skb when it is called
> in vlan_untag, lead to a memory leak.
> 
> Signed-off-by: Li RongQing <roy.qing.li@gmail.com>

Applied and queued up for -stable, thanks.

And I agree with Eric that we could define this to return error codes
since 'skb' never changes so returning skb vs. NULL is rather awkward.