linux-kernel - Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

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: <20200224122206.10984b3f@collabora.com>
Date:   Mon, 24 Feb 2020 12:22:06 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Vitor Soares <Vitor.Soares@...opsys.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
        Jose Abreu <Jose.Abreu@...opsys.com>,
        "corbet@....net" <corbet@....net>,
        Joao Pinto <Joao.Pinto@...opsys.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "wsa@...-dreams.de" <wsa@...-dreams.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "bbrezillon@...nel.org" <bbrezillon@...nel.org>,
        "broonie@...nel.org" <broonie@...nel.org>
Subject: Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev

On Mon, 24 Feb 2020 11:04:50 +0000
Vitor Soares <Vitor.Soares@...opsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@...labora.com>
> Date: Fri, Feb 21, 2020 at 22:32:16
> 
> > On Wed, 19 Feb 2020 01:20:42 +0100
> > Vitor Soares <Vitor.Soares@...opsys.com> wrote:
> >   
> > > +static int i3cdev_detach(struct device *dev, void *dummy)
> > > +{
> > > +	struct i3cdev_data *i3cdev;
> > > +	struct i3c_device *i3c;
> > > +
> > > +	if (dev->type == &i3c_masterdev_type)
> > > +		return 0;
> > > +
> > > +	i3c = dev_to_i3cdev(dev);
> > > +
> > > +	i3cdev = i3cdev_get_drvdata(i3c);
> > > +	if (!i3cdev)
> > > +		return 0;
> > > +
> > > +	/* Prevent transfers while cdev removal */
> > > +	mutex_lock(&i3cdev->xfer_lock);
> > > +	cdev_del(&i3cdev->cdev);  
> > 
> > When cdev_del() returns there might be opened FDs pointing to your
> > i3cdev [1] ...  
> 
> Yes, I know. I protected the driver part but I missed the 
> file->private_data.

Not sure what you mean by protection, but if you meant locking, then
it's not enough: you need to refcnt the struct if you want to prevent
use-after-free situations.

BTW, I had a closer look at the usbdev implementation, and maybe you
should base yours on usb instead of i2c. They seem to register a cdev at
module init time, and add a dev_t per device at device registration
time. Not sure how they handle the userspace-driver vs kernel-driver
concurrency, but maybe returning EACCES (or EBUSY) instead of
detaching/attaching the i3cdev everytime a device is bound/unbound
would be simpler.

Also, I think Arnd was right, it'd be simpler if i3cdev support was
integrated to the core (still left as a option so it can be disabled,
but with a dedicated i3cdev field in i3c_device instead hijacking the
driver private field).