netdev - Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim operations

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200112061937.171f6d88@cakuba>
Date:   Sun, 12 Jan 2020 06:19:37 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim
 operations

On Sat, 11 Jan 2020 16:36:55 +0000, Taehee Yoo wrote:
> netdevsim basic operations are called with sysfs.
> 
> Create netdevsim device:
> echo "1 1" > /sys/bus/netdevsim/new_device
> Delete netdevsim device:
> echo 1 > /sys/bus/netdevsim/del_device
> Create netdevsim port:
> echo 4 > /sys/devices/netdevsim1/new_port
> Delete netdevsim port:
> echo 4 > /sys/devices/netdevsim1/del_port
> Set sriov number:
> echo 4 > /sys/devices/netdevsim1/sriov_numvfs
> 
> These operations use the same resource so they should acquire a lock for
> the whole resource not only for a list.
> 
> Test commands:
>     #SHELL1
>     modprobe netdevsim
>     while :
>     do
>         echo "1 1" > /sys/bus/netdevsim/new_device
>         echo "1 1" > /sys/bus/netdevsim/del_device
>     done
> 
>     #SHELL2
>     while :
>     do
>         echo 1 > /sys/devices/netdevsim1/new_port
>         echo 1 > /sys/devices/netdevsim1/del_port
>     done
> 
> Splat looks like:
> [  151.623634][ T1165] kasan: CONFIG_KASAN_INLINE enabled
> [  151.626503][ T1165] kasan: GPF could be caused by NULL-ptr deref or user memory access


> In this patch, __init and __exit function also acquire a lock.
> operations could be called while __init and __exit functions are
> processing. If so, uninitialized or freed resources could be used.
> So, __init() and __exit function also need lock.
> 
> Fixes: 83c9e13aa39a ("netdevsim: add software driver for testing offloads")

I don't think that's the correct Fixes tag, the first version of the
driver did not use the sysfs interface.

> Signed-off-by: Taehee Yoo <ap420073@...il.com>

> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -16,7 +16,8 @@
>  
>  static DEFINE_IDA(nsim_bus_dev_ids);
>  static LIST_HEAD(nsim_bus_dev_list);
> -static DEFINE_MUTEX(nsim_bus_dev_list_lock);
> +/* mutex lock for netdevsim operations */
> +DEFINE_MUTEX(nsim_bus_dev_ops_lock);
>  
>  static struct nsim_bus_dev *to_nsim_bus_dev(struct device *dev)
>  {
> @@ -51,9 +52,14 @@ nsim_bus_dev_numvfs_store(struct device *dev, struct device_attribute *attr,

Could the vf handling use the device lock like PCI does?

But actually, we free VF info from the release function, which IIUC is
called after all references to the device are gone. So this should be
fine, no?

Perhaps the entire bus dev structure should be freed from release?

>  	unsigned int num_vfs;
>  	int ret;
>  
> +	if (!mutex_trylock(&nsim_bus_dev_ops_lock))
> +		return -EBUSY;

Why the trylocks? Are you trying to catch the races between unregister
and other ops?

>  	ret = kstrtouint(buf, 0, &num_vfs);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&nsim_bus_dev_ops_lock);
>  		return ret;
> +	}
>  
>  	rtnl_lock();
>  	if (nsim_bus_dev->num_vfs == num_vfs)