linux-kernel - Re: [PATCH] drivers: core: Make dev->driver usage safe 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: <20240430081754.GA1927@mypc>
Date: Tue, 30 Apr 2024 10:17:54 +0200
From: Eugeniu Rosca <erosca@...adit-jv.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Dirk Behme <dirk.behme@...bosch.com>, <linux-kernel@...r.kernel.org>,
	Rafael J Wysocki <rafael@...nel.org>,
	<syzbot+ffa8143439596313a85a@...kaller.appspotmail.com>, Eugeniu Rosca
	<eugeniu.rosca@...ch.com>, Eugeniu Rosca <erosca@...adit-jv.com>, Eugeniu
 Rosca <roscaeugeniu@...il.com>
Subject: Re: [PATCH] drivers: core: Make dev->driver usage safe in
 dev_uevent()

Hi Greg,

On Tue, Apr 30, 2024 at 09:20:10AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 30, 2024 at 06:55:31AM +0200, Dirk Behme wrote:
> > Inspired by the function dev_driver_string() in the same file make sure
> > in case of uninitialization dev->driver is used safely in dev_uevent(),
> > as well.
> 
> I think you are racing and just getting "lucky" with your change here,
> just like dev_driver_string() is doing there (that READ_ONCE() really
> isn't doing much to protect you...)

I hope below details shed more details on the repro:
https://gist.github.com/erosca/1e8a87fbcc9e5ad0fecd32ebcb6266c3

To improve the occurrence rate:
 - a dummy ds90ux9xx-dummy driver was used
 - a dummy i2c node was added to DTS
 - a dummy pr_alert() was added to dev_uevent() @ drivers/base/core.c
 - UBSAN + KASAN enabled in .config

> > This change is based on the observation of the following race condition:
> > 
> > Thread #1:
> > ==========
> > 
> > really_probe() {
> > ...
> > probe_failed:
> > ...
> > device_unbind_cleanup(dev) {
> >       ...
> >       dev->driver = NULL;   // <= Failed probe sets dev->driver to NULL
> >       ...
> >       }
> > ...
> > }
> > 
> > Thread #2:
> > ==========
> > 
> > dev_uevent() {
> 
> Wait, how can dev_uevent() be called if probe fails?  Who is calling
> that?

dev_uevent() is called by reading /sys/bus/i2c/devices/<dev>/uevent.
Not directly triggered by the probe failure.
Please, kindly check the above gist/notes.

[--- cut ---]

> > -	if (dev->driver)
> > -		add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> > +	/* dev->driver can change to NULL underneath us because of unbinding
> > +	 * or failing probe(), so be careful about accessing it.
> > +	 */
> > +	drv = READ_ONCE(dev->driver);
> > +	if (drv)
> > +		add_uevent_var(env, "DRIVER=%s", drv->name);
> 
> Again, you are just reducing the window here.  Maybe a bit, but not all
> that much overall as there is no real lock present.

The main objective of the patch is to "cache" dev->driver, such
that it is not cleared asynchronously from a parallel thread.
A refined/minimal locking alternative (if feasible) is welcome.

> 
> So how is this actually solving anything?  And who is calling a uevent
> on a device that is not probed properly?  Userspace?  Within the kernel?
> Something else?

Repro details provided in the gist/notes above.

BR, Eugeniu