[net] vlan: fix a use-after-free in vlan_device_event()

Message ID 20171110004313.20662-1-xiyou.wangcong@gmail.com
State Accepted
Delegated to: David Miller
Headers show
Series
  • [net] vlan: fix a use-after-free in vlan_device_event()
Related show

Commit Message

Cong Wang Nov. 10, 2017, 12:43 a.m.
After refcnt reaches zero, vlan_vid_del() could free
dev->vlan_info via RCU:

	RCU_INIT_POINTER(dev->vlan_info, NULL);
	call_rcu(&vlan_info->rcu, vlan_info_rcu_free);

However, the pointer 'grp' still points to that memory
since it is set before vlan_vid_del():

        vlan_info = rtnl_dereference(dev->vlan_info);
        if (!vlan_info)
                goto out;
        grp = &vlan_info->grp;

Depends on when that RCU callback is scheduled, we could
trigger a use-after-free in vlan_group_for_each_dev()
right following this vlan_vid_del().

Fix it by moving vlan_vid_del() before setting grp. This
is also symmetric to the vlan_vid_add() we call in
vlan_device_event().

Reported-by: Fengguang Wu <fengguang.wu@intel.com>
Fixes: efc73f4bbc23 ("net: Fix memory leak - vlan_info struct")
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Girish Moodalbail <girish.moodalbail@oracle.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/8021q/vlan.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Girish Moodalbail Nov. 10, 2017, 1:46 a.m. | #1
On 11/9/17 4:43 PM, Cong Wang wrote:
> After refcnt reaches zero, vlan_vid_del() could free
> dev->vlan_info via RCU:
> 
> 	RCU_INIT_POINTER(dev->vlan_info, NULL);
> 	call_rcu(&vlan_info->rcu, vlan_info_rcu_free);
> 
> However, the pointer 'grp' still points to that memory
> since it is set before vlan_vid_del():
> 
>          vlan_info = rtnl_dereference(dev->vlan_info);
>          if (!vlan_info)
>                  goto out;
>          grp = &vlan_info->grp;
> 
> Depends on when that RCU callback is scheduled, we could
> trigger a use-after-free in vlan_group_for_each_dev()
> right following this vlan_vid_del().
> 
> Fix it by moving vlan_vid_del() before setting grp. This
> is also symmetric to the vlan_vid_add() we call in
> vlan_device_event().
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Fixes: efc73f4bbc23 ("net: Fix memory leak - vlan_info struct")
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Girish Moodalbail <girish.moodalbail@oracle.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

LGTM.

Reviewed-by: Girish Moodalbail <girish.moodalbail@oracle.com>

Thanks,
~Girish


> ---
>   net/8021q/vlan.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
> index 9649579b5b9f..4a72ee4e2ae9 100644
> --- a/net/8021q/vlan.c
> +++ b/net/8021q/vlan.c
> @@ -376,6 +376,9 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>   			dev->name);
>   		vlan_vid_add(dev, htons(ETH_P_8021Q), 0);
>   	}
> +	if (event == NETDEV_DOWN &&
> +	    (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
> +		vlan_vid_del(dev, htons(ETH_P_8021Q), 0);
>   
>   	vlan_info = rtnl_dereference(dev->vlan_info);
>   	if (!vlan_info)
> @@ -423,9 +426,6 @@ static int vlan_device_event(struct notifier_block *unused, unsigned long event,
>   		struct net_device *tmp;
>   		LIST_HEAD(close_list);
>   
> -		if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
> -			vlan_vid_del(dev, htons(ETH_P_8021Q), 0);
> -
>   		/* Put all VLANs for this dev in the down state too.  */
>   		vlan_group_for_each_dev(grp, i, vlandev) {
>   			flgs = vlandev->flags;
>
Fengguang Wu Nov. 10, 2017, 11:50 a.m. | #2
It works, thank you for fixing this ancient bug!

Tested-by: Fengguang Wu <fengguang.wu@intel.com>
Linus Torvalds Nov. 10, 2017, 7:48 p.m. | #3
On Fri, Nov 10, 2017 at 3:50 AM, Fengguang Wu <fengguang.wu@intel.com> wrote:
> It works, thank you for fixing this ancient bug!
>
> Tested-by: Fengguang Wu <fengguang.wu@intel.com>

Thanks for all the 0day work to make people finally figure this out.

             Linus
David Miller Nov. 11, 2017, 10:36 a.m. | #4
From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Thu,  9 Nov 2017 16:43:13 -0800

> After refcnt reaches zero, vlan_vid_del() could free
> dev->vlan_info via RCU:
> 
> 	RCU_INIT_POINTER(dev->vlan_info, NULL);
> 	call_rcu(&vlan_info->rcu, vlan_info_rcu_free);
> 
> However, the pointer 'grp' still points to that memory
> since it is set before vlan_vid_del():
> 
>         vlan_info = rtnl_dereference(dev->vlan_info);
>         if (!vlan_info)
>                 goto out;
>         grp = &vlan_info->grp;
> 
> Depends on when that RCU callback is scheduled, we could
> trigger a use-after-free in vlan_group_for_each_dev()
> right following this vlan_vid_del().
> 
> Fix it by moving vlan_vid_del() before setting grp. This
> is also symmetric to the vlan_vid_add() we call in
> vlan_device_event().
> 
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> Fixes: efc73f4bbc23 ("net: Fix memory leak - vlan_info struct")
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Girish Moodalbail <girish.moodalbail@oracle.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>

Applied and queued up for -stable, thanks Cong!

Patch

diff --git a/net/8021q/vlan.c b/net/8021q/vlan.c
index 9649579b5b9f..4a72ee4e2ae9 100644
--- a/net/8021q/vlan.c
+++ b/net/8021q/vlan.c
@@ -376,6 +376,9 @@  static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 			dev->name);
 		vlan_vid_add(dev, htons(ETH_P_8021Q), 0);
 	}
+	if (event == NETDEV_DOWN &&
+	    (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER))
+		vlan_vid_del(dev, htons(ETH_P_8021Q), 0);
 
 	vlan_info = rtnl_dereference(dev->vlan_info);
 	if (!vlan_info)
@@ -423,9 +426,6 @@  static int vlan_device_event(struct notifier_block *unused, unsigned long event,
 		struct net_device *tmp;
 		LIST_HEAD(close_list);
 
-		if (dev->features & NETIF_F_HW_VLAN_CTAG_FILTER)
-			vlan_vid_del(dev, htons(ETH_P_8021Q), 0);
-
 		/* Put all VLANs for this dev in the down state too.  */
 		vlan_group_for_each_dev(grp, i, vlandev) {
 			flgs = vlandev->flags;