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 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]

Date:   Wed, 15 Jan 2020 06:16:34 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Taehee Yoo <ap420073@...il.com>
Cc:     Jakub Kicinski <jakub.kicinski@...ronome.com>,
        David Miller <davem@...emloft.net>,
        Netdev <netdev@...r.kernel.org>, Jiri Pirko <jiri@...nulli.us>
Subject: Re: [PATCH net 1/5] netdevsim: fix a race condition in netdevsim
 operations

On Wed, 15 Jan 2020 00:26:22 +0900, Taehee Yoo wrote:
> On Sun, 12 Jan 2020 at 23:19, Jakub Kicinski wrote:
> Hi Jakub,
> Thank you for your review!

Thank you for fixing these tricky bugs! :)

> > Perhaps the entire bus dev structure should be freed from release?
> 
> I tested this like this.
> 
> @@ -146,6 +161,8 @@ static void nsim_bus_dev_release(struct device *dev)
>         struct nsim_bus_dev *nsim_bus_dev = to_nsim_bus_dev(dev);
> 
>         nsim_bus_dev_vfs_disable(nsim_bus_dev);
> +       ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
> +       kfree(nsim_bus_dev);
>  }
> @@ -300,8 +320,6 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count)
>  static void nsim_bus_dev_del(struct nsim_bus_dev *nsim_bus_dev)
>  {
>         device_unregister(&nsim_bus_dev->dev);
> -       ida_free(&nsim_bus_dev_ids, nsim_bus_dev->dev.id);
> -       kfree(nsim_bus_dev);
>  }
> 
> It works well. but I'm not sure this is needed.

My concern is that process A opens a sysfs file (eg. numvfs) then
process B deletes the device, but process A still has a file descriptor
(and device reference) so it may be able to write/read the numvfs file
even though nsim_bus_dev was already freed. 

I may very well be wrong, and something else may be preventing this
condition. It's just a bit strange to see release free an internal
sub-structure, while the main structure is freed immediately..

> > >       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?
> >  
> 
> The reason is to avoid deadlock.
> If we use mutex_lock() instead of mutex_trylock(), the below message
> will be printed and actual deadlock also appeared.

> [  426.907883][  T805]  Possible unsafe locking scenario:
> [  426.907883][  T805]
> [  426.908715][  T805]        CPU0                    CPU1
> [  426.909312][  T805]        ----                    ----
> [  426.909902][  T805]   lock(kn->count#170);
> [  426.910372][  T805]
> lock(nsim_bus_dev_ops_lock);
> [  426.911277][  T805]                                lock(kn->count#170);
> [  426.912032][  T805]   lock(nsim_bus_dev_ops_lock);

> Locking ordering of {new/del}_device() and {new/del}_port is different.
> So, a deadlock could occur.

Hm, we can't use the same lock for the bus ops and port ops.
But the port ops already take port lock, do we really need 
another lock there?

Also does nsim_bus_exit() really need to iterate over devices to remove
them? Does core not do it for us?