Re: [PATCH][NETNS]: Fix arbitrary net_device-s corruptions on net_ns stop.

[Daniel Lezcano <dlezcano@fr.ibm.com>]

Pavel Emelyanov wrote:
> When a net namespace is destroyed, some devices (those, not killed
> on ns stop explicitly) are moved back to init_net.
> 
> The problem, is that this net_ns change has one point of failure -
> the __dev_alloc_name() may be called if a name collision occurs (and
> this is easy to trigger). This allocator performs a likely-to-fail
> GFP_ATOMIC allocation to find a suitable number. Other possible 
> conditions that may cause error (for device being ns local or not
> registered) are always false in this case.
> 
> So, when this call fails, the device is unregistered. But this is
> *not* the right thing to do, since after this the device may be
> released (and kfree-ed) improperly. E. g. bridges require more
> actions (sysfs update, timer disarming, etc.), some other devices 
> want to remove their private areas from lists, etc.
> 
> I. e. arbitrary use-after-free cases may occur.
> 
> The proposed fix is the following: since the only reason for the
> dev_change_net_namespace to fail is the name generation, we may
> give it a unique fall-back name w/o %d-s in it - the dev<ifindex>
> one, since ifindexes are still unique.
> 
> So make this change, raise the failure-case printk loglevel to 
> EMERG and replace the unregister_netdevice call with BUG().
> 
> Signed-off-by: Pavel Emelyanov <xemul@openvz.org>
> 
> ---

Good catch :)

> diff --git a/net/core/dev.c b/net/core/dev.c
> index d334446..86da6aa 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4480,17 +4480,19 @@ static void __net_exit default_device_exit(struct net *net)
>  	rtnl_lock();
>  	for_each_netdev_safe(net, dev, next) {
>  		int err;
> +		char fb_name[IFNAMSIZ];
> 
>  		/* Ignore unmoveable devices (i.e. loopback) */
>  		if (dev->features & NETIF_F_NETNS_LOCAL)
>  			continue;
> 
>  		/* Push remaing network devices to init_net */
> -		err = dev_change_net_namespace(dev, &init_net, "dev%d");
> +		sprintf(fb_name, "dev%d", dev->ifindex);

The computed interface name can not exceed IFNAMSIZ, 3 ('dev') + 10 (max 
int) + 1 ('\0'). In this case there is no risk to corrupt the stack but 
may be it is more secure to change that to snprintf(fb_name, IFNAMSIZ, 
"dev%d", dev->ifindex), just in case, no ?

> +		err = dev_change_net_namespace(dev, &init_net, fb_name);
>  		if (err) {
> -			printk(KERN_WARNING "%s: failed to move %s to init_net: %d\n",
> +			printk(KERN_EMERG "%s: failed to move %s to init_net: %d\n",
>  				__func__, dev->name, err);
> -			unregister_netdevice(dev);
> +			BUG();
>  		}
>  	}
>  	rtnl_unlock();
> --
> 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
>