drivers/base/core.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
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.
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() {
...
if (dev->driver)
// If dev->driver is NULLed from really_probe() from here on,
// after above check, the system crashes
add_uevent_var(env, "DRIVER=%s", dev->driver->name);
dev_driver_string() can't be used here because we want NULL and not
anything else in the non-init case.
Similar cases are reported by syzkaller in
https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
But these are regarding the *initialization* of dev->driver
dev->driver = drv;
As this switches dev->driver to non-NULL these reports can be considered
to be false-positives (which should be "fixed" by this commit, as well,
though).
Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
---
drivers/base/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 5f4e03336e68..99ead727c08f 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
{
const struct device *dev = kobj_to_dev(kobj);
+ struct device_driver *drv;
int retval = 0;
/* add device node properties if present */
@@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
if (dev->type && dev->type->name)
add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
- 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);
/* Add common DT information about the device */
of_device_uevent(dev, env);
--
2.28.0On 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...)
> 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?
> ...
> if (dev->driver)
> // If dev->driver is NULLed from really_probe() from here on,
> // after above check, the system crashes
> add_uevent_var(env, "DRIVER=%s", dev->driver->name);
>
> dev_driver_string() can't be used here because we want NULL and not
> anything else in the non-init case.
>
> Similar cases are reported by syzkaller in
>
> https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
>
> But these are regarding the *initialization* of dev->driver
>
> dev->driver = drv;
>
> As this switches dev->driver to non-NULL these reports can be considered
> to be false-positives (which should be "fixed" by this commit, as well,
> though).
>
> Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
> Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
> Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> ---
> drivers/base/core.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5f4e03336e68..99ead727c08f 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
> static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> {
> const struct device *dev = kobj_to_dev(kobj);
> + struct device_driver *drv;
> int retval = 0;
>
> /* add device node properties if present */
> @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> if (dev->type && dev->type->name)
> add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>
> - 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.
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?
thanks,
greg k-h
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
On Tue, Apr 30, 2024 at 10:17:54AM +0200, Eugeniu Rosca wrote:
> 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
Sometimes I only have access to email, nothing else, please include in
the email the full details.
> 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
So this is entirely fake? No real device or driver ever causes this
problem?
Why would you add a pr_alert() call? What is that for?
totally confused.
>
> > > 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.
Again, put the info in the email so we can properly quote and read it,
and it's present for the history involved (web pages disappear, email is
for forever.)
So you have userspace hammering on a uevent file? Why is it being
called if userspace hasn't even been notified that the device has a
driver bound to it yet? What causes this action?
>
> [--- 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.
"cacheing" a stale pointer still results in a stale pointer :(
thanks,
greg k-h
Hello Greg,
On Tue, Apr 30, 2024 at 10:27:19AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 30, 2024 at 10:17:54AM +0200, Eugeniu Rosca wrote:
> > 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
>
> Sometimes I only have access to email, nothing else, please include in
> the email the full details.
Will follow your preference in the future.
>
> > 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
>
> So this is entirely fake? No real device or driver ever causes this
> problem?
Of course not. This issue is impacting the end user by resetting the HW
target once in a couple of months. Our synthetic test-case tries to
emulate the end user's scenario, for quicker reproduction & validation
of potential/candidate solutions.
Dirk's synthetic scenario leads to the same logs as shared by the user.
Based on that evidence, we believe we found the root cause.
>
> Why would you add a pr_alert() call? What is that for?
>
> totally confused.
pr_alert() acts as a simple delay, accelerating the reproduction.
>
>
> >
> > > > 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.
>
> Again, put the info in the email so we can properly quote and read it,
> and it's present for the history involved (web pages disappear, email is
> for forever.)
Agreed & will follow in the future (did not want to clutter the e-mail)
>
> So you have userspace hammering on a uevent file? Why is it being
> called if userspace hasn't even been notified that the device has a
> driver bound to it yet? What causes this action?
We know that uevent subsystem is involved, based on the post-mortem logs.
Hence, reading sysfs was the easiest way to translate the real-life
use-case to a synthetic one.
> >
> > [--- 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.
>
> "cacheing" a stale pointer still results in a stale pointer :(
Agreed. So, likely minimal/least-intrusive locking will be necessary.
>
> thanks,
>
> greg k-h
BR, Eugeniu
Hi Greg,
On 30.04.2024 09:20, 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...)
>
>> 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?
>
>> ...
>> if (dev->driver)
>> // If dev->driver is NULLed from really_probe() from here on,
>> // after above check, the system crashes
>> add_uevent_var(env, "DRIVER=%s", dev->driver->name);
>>
>> dev_driver_string() can't be used here because we want NULL and not
>> anything else in the non-init case.
>>
>> Similar cases are reported by syzkaller in
>>
>> https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
>>
>> But these are regarding the *initialization* of dev->driver
>>
>> dev->driver = drv;
>>
>> As this switches dev->driver to non-NULL these reports can be considered
>> to be false-positives (which should be "fixed" by this commit, as well,
>> though).
>>
>> Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
>> Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
>> Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>> Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>> ---
>> drivers/base/core.c | 9 +++++++--
>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index 5f4e03336e68..99ead727c08f 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>> static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>> {
>> const struct device *dev = kobj_to_dev(kobj);
>> + struct device_driver *drv;
>> int retval = 0;
>>
>> /* add device node properties if present */
>> @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>> if (dev->type && dev->type->name)
>> add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>>
>> - 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.
>
> So how is this actually solving anything?
Looking at dev_driver_string() I hoped that it just reads *once*. I.e.
we don't care if we read NULL or any valid pointer, as long as this
pointer read is done only once ("atomically"?). If READ_ONCE() doesn't
do that, I agree, it's not the (race) fix we are looking for.
Initially, I was thinking about anything like [1] below. I.e. adding a
mutex lock. But not sure if that is better (acceptable?).
> And who is calling a uevent
> on a device that is not probed properly? Userspace?
To my understanding, yes, user space. The mentioned syzkaller has the
callstack [2]. To my understanding a dev_info()/dev_err() in the failing
probe() does trigger systemd-journal/udevd to write that to a log (?).
We are using a (I2C) test module probe() like [3] to trigger this issue.
If you iterate through the delays you might find a "window" to hit this
race. Usually, we found a delay between 1 - 2 ms for that.
Best regards
Dirk
[1]
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a1d3b2a043f..45c6edd90122 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -900,6 +900,7 @@ static int dev_uevent(struct kset *kset, struct
kobject *kobj,
struct kobj_uevent_env *env)
{
struct device *dev = kobj_to_dev(kobj);
+ const char *driver_name = NULL;
int retval = 0;
/* add device node properties if present */
@@ -928,8 +929,13 @@ static int dev_uevent(struct kset *kset, struct
kobject *kobj,
if (dev->type && dev->type->name)
add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
+ /* Synchronization with really_probe() modifying dev->driver */
+ device_lock(dev);
if (dev->driver)
- add_uevent_var(env, "DRIVER=%s", dev->driver->name);
+ driver_name = dev->driver->name;
+ device_unlock(dev);
+ if (driver_name)
+ add_uevent_var(env, "DRIVER=%s", driver_name);
/* Add common DT information about the device */
of_device_uevent(dev, env);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6143bf085e94..176dc8cd0bb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -400,7 +400,9 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
}
re_probe:
+ device_lock(dev);
dev->driver = drv;
+ device_unlock(dev);
/* If using pinctrl, bind pins now before probing */
ret = pinctrl_bind_pins(dev);
@@ -472,7 +474,9 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
devres_release_all(dev);
dma_deconfigure(dev);
driver_sysfs_remove(dev);
+ device_lock(dev);
dev->driver = NULL;
+ device_unlock(dev);
dev_set_drvdata(dev, NULL);
if (dev->pm_domain && dev->pm_domain->dismiss)
dev->pm_domain->dismiss(dev);
[2]
read to 0xffff88811759c468 of 8 bytes by task 3901 on cpu 1:
dev_uevent+0x235/0x380 drivers/base/core.c:2670
uevent_show+0x10c/0x1f0 drivers/base/core.c:2742
dev_attr_show+0x3a/0xa0 drivers/base/core.c:2445
sysfs_kf_seq_show+0x17c/0x250 fs/sysfs/file.c:59
kernfs_seq_show+0x7c/0x90 fs/kernfs/file.c:205
seq_read_iter+0x2d7/0x940 fs/seq_file.c:230
kernfs_fop_read_iter+0xc6/0x310 fs/kernfs/file.c:279
call_read_iter include/linux/fs.h:2104 [inline]
new_sync_read fs/read_write.c:395 [inline]
vfs_read+0x5bc/0x6b0 fs/read_write.c:476
ksys_read+0xeb/0x1b0 fs/read_write.c:619
__do_sys_read fs/read_write.c:629 [inline]
__se_sys_read fs/read_write.c:627 [inline]
__x64_sys_read+0x42/0x50 fs/read_write.c:627
x64_sys_call+0x27ad/0x2d30 arch/x86/include/generated/asm/syscalls_64.h:1
do_syscall_x64 arch/x86/entry/common.c:52 [inline]
do_syscall_64+0xcd/0x1d0 arch/x86/entry/common.c:83
entry_SYSCALL_64_after_hwframe+0x77/0x7f
[3]
static int waitms = 0;
module_param(waitms, int, 0660);
MODULE_PARM_DESC(waitms, "delay time in ms. If no value is given there
is no delay (0ms)");
static int waitus = 0;
module_param(waitus, int, 0660);
MODULE_PARM_DESC(waitus, "delay time in us. If no value is given there
is no delay (0ms)");
static int i2c_dummy_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
int ret = -ENXIO;
i2c_set_clientdata(client, NULL);
if (waitms)
dev_info(&client->dev, "probe() called. waiting %dms\n", waitms);
if (waitus)
dev_info(&client->dev, "probe() called. waiting %dus\n", waitus);
if (waitms)
msleep(waitms);
if (waitus)
udelay(waitus);
/* failure: */
/* We intentionally want probe() to return with failure */
i2c_set_clientdata(client, NULL);
dev_err(&client->dev, "Error: probe failed with %d\n", ret);
return ret;
}
On Tue, Apr 30, 2024 at 10:23:36AM +0200, Dirk Behme wrote:
> Hi Greg,
>
> On 30.04.2024 09:20, 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...)
> >
> > > 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?
> >
> > > ...
> > > if (dev->driver)
> > > // If dev->driver is NULLed from really_probe() from here on,
> > > // after above check, the system crashes
> > > add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> > >
> > > dev_driver_string() can't be used here because we want NULL and not
> > > anything else in the non-init case.
> > >
> > > Similar cases are reported by syzkaller in
> > >
> > > https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
> > >
> > > But these are regarding the *initialization* of dev->driver
> > >
> > > dev->driver = drv;
> > >
> > > As this switches dev->driver to non-NULL these reports can be considered
> > > to be false-positives (which should be "fixed" by this commit, as well,
> > > though).
> > >
> > > Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
> > > Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
> > > Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> > > Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> > > ---
> > > drivers/base/core.c | 9 +++++++--
> > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > index 5f4e03336e68..99ead727c08f 100644
> > > --- a/drivers/base/core.c
> > > +++ b/drivers/base/core.c
> > > @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
> > > static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> > > {
> > > const struct device *dev = kobj_to_dev(kobj);
> > > + struct device_driver *drv;
> > > int retval = 0;
> > > /* add device node properties if present */
> > > @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> > > if (dev->type && dev->type->name)
> > > add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
> > > - 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.
> >
> > So how is this actually solving anything?
>
>
> Looking at dev_driver_string() I hoped that it just reads *once*. I.e. we
> don't care if we read NULL or any valid pointer, as long as this pointer
> read is done only once ("atomically"?). If READ_ONCE() doesn't do that, I
> agree, it's not the (race) fix we are looking for.
Yes, what if you read it, and then it is unloaded from the system before
you attempt to access drv->name? not good.
> Initially, I was thinking about anything like [1] below. I.e. adding a mutex
> lock. But not sure if that is better (acceptable?).
a proper lock is the only way to correctly solve this.
thanks,
greg k-h
On 30.04.2024 10:41, Greg Kroah-Hartman wrote:
> On Tue, Apr 30, 2024 at 10:23:36AM +0200, Dirk Behme wrote:
>> Hi Greg,
>>
>> On 30.04.2024 09:20, 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...)
>>>
>>>> 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?
>>>
>>>> ...
>>>> if (dev->driver)
>>>> // If dev->driver is NULLed from really_probe() from here on,
>>>> // after above check, the system crashes
>>>> add_uevent_var(env, "DRIVER=%s", dev->driver->name);
>>>>
>>>> dev_driver_string() can't be used here because we want NULL and not
>>>> anything else in the non-init case.
>>>>
>>>> Similar cases are reported by syzkaller in
>>>>
>>>> https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
>>>>
>>>> But these are regarding the *initialization* of dev->driver
>>>>
>>>> dev->driver = drv;
>>>>
>>>> As this switches dev->driver to non-NULL these reports can be considered
>>>> to be false-positives (which should be "fixed" by this commit, as well,
>>>> though).
>>>>
>>>> Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
>>>> Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
>>>> Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>>>> Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>> ---
>>>> drivers/base/core.c | 9 +++++++--
>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>> index 5f4e03336e68..99ead727c08f 100644
>>>> --- a/drivers/base/core.c
>>>> +++ b/drivers/base/core.c
>>>> @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>>>> static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>>>> {
>>>> const struct device *dev = kobj_to_dev(kobj);
>>>> + struct device_driver *drv;
>>>> int retval = 0;
>>>> /* add device node properties if present */
>>>> @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>>>> if (dev->type && dev->type->name)
>>>> add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>>>> - 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.
>>>
>>> So how is this actually solving anything?
>>
>>
>> Looking at dev_driver_string() I hoped that it just reads *once*. I.e. we
>> don't care if we read NULL or any valid pointer, as long as this pointer
>> read is done only once ("atomically"?). If READ_ONCE() doesn't do that, I
>> agree, it's not the (race) fix we are looking for.
>
> Yes, what if you read it, and then it is unloaded from the system before
> you attempt to access drv->name? not good.
>
>> Initially, I was thinking about anything like [1] below. I.e. adding a mutex
>> lock. But not sure if that is better (acceptable?).
>
> a proper lock is the only way to correctly solve this.
Would using device_lock()/unlock() for locking like done below [1]
acceptable?
Best regards
Dirk
[1]
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 2a1d3b2a043f..45c6edd90122 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -900,6 +900,7 @@ static int dev_uevent(struct kset *kset, struct
kobject *kobj,
struct kobj_uevent_env *env)
{
struct device *dev = kobj_to_dev(kobj);
+ const char *driver_name = NULL;
int retval = 0;
/* add device node properties if present */
@@ -928,8 +929,13 @@ static int dev_uevent(struct kset *kset, struct
kobject *kobj,
if (dev->type && dev->type->name)
add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
+ /* Synchronization with really_probe() modifying dev->driver */
+ device_lock(dev);
if (dev->driver)
- add_uevent_var(env, "DRIVER=%s", dev->driver->name);
+ driver_name = dev->driver->name;
+ device_unlock(dev);
+ if (driver_name)
+ add_uevent_var(env, "DRIVER=%s", driver_name);
/* Add common DT information about the device */
of_device_uevent(dev, env);
diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 6143bf085e94..176dc8cd0bb1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -400,7 +400,9 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
}
re_probe:
+ device_lock(dev);
dev->driver = drv;
+ device_unlock(dev);
/* If using pinctrl, bind pins now before probing */
ret = pinctrl_bind_pins(dev);
@@ -472,7 +474,9 @@ static int really_probe(struct device *dev, struct
device_driver *drv)
devres_release_all(dev);
dma_deconfigure(dev);
driver_sysfs_remove(dev);
+ device_lock(dev);
dev->driver = NULL;
+ device_unlock(dev);
dev_set_drvdata(dev, NULL);
if (dev->pm_domain && dev->pm_domain->dismiss)
dev->pm_domain->dismiss(dev);
On Tue, Apr 30, 2024 at 10:50:52AM +0200, Dirk Behme wrote:
> On 30.04.2024 10:41, Greg Kroah-Hartman wrote:
> > On Tue, Apr 30, 2024 at 10:23:36AM +0200, Dirk Behme wrote:
> > > Hi Greg,
> > >
> > > On 30.04.2024 09:20, 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...)
> > > >
> > > > > 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?
> > > >
> > > > > ...
> > > > > if (dev->driver)
> > > > > // If dev->driver is NULLed from really_probe() from here on,
> > > > > // after above check, the system crashes
> > > > > add_uevent_var(env, "DRIVER=%s", dev->driver->name);
> > > > >
> > > > > dev_driver_string() can't be used here because we want NULL and not
> > > > > anything else in the non-init case.
> > > > >
> > > > > Similar cases are reported by syzkaller in
> > > > >
> > > > > https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
> > > > >
> > > > > But these are regarding the *initialization* of dev->driver
> > > > >
> > > > > dev->driver = drv;
> > > > >
> > > > > As this switches dev->driver to non-NULL these reports can be considered
> > > > > to be false-positives (which should be "fixed" by this commit, as well,
> > > > > though).
> > > > >
> > > > > Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
> > > > > Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
> > > > > Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> > > > > Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
> > > > > Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
> > > > > ---
> > > > > drivers/base/core.c | 9 +++++++--
> > > > > 1 file changed, 7 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/core.c b/drivers/base/core.c
> > > > > index 5f4e03336e68..99ead727c08f 100644
> > > > > --- a/drivers/base/core.c
> > > > > +++ b/drivers/base/core.c
> > > > > @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
> > > > > static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> > > > > {
> > > > > const struct device *dev = kobj_to_dev(kobj);
> > > > > + struct device_driver *drv;
> > > > > int retval = 0;
> > > > > /* add device node properties if present */
> > > > > @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> > > > > if (dev->type && dev->type->name)
> > > > > add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
> > > > > - 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.
> > > >
> > > > So how is this actually solving anything?
> > >
> > >
> > > Looking at dev_driver_string() I hoped that it just reads *once*. I.e. we
> > > don't care if we read NULL or any valid pointer, as long as this pointer
> > > read is done only once ("atomically"?). If READ_ONCE() doesn't do that, I
> > > agree, it's not the (race) fix we are looking for.
> >
> > Yes, what if you read it, and then it is unloaded from the system before
> > you attempt to access drv->name? not good.
> >
> > > Initially, I was thinking about anything like [1] below. I.e. adding a mutex
> > > lock. But not sure if that is better (acceptable?).
> >
> > a proper lock is the only way to correctly solve this.
>
>
> Would using device_lock()/unlock() for locking like done below [1]
> acceptable?
Why not test it out and see! :)
On 30.04.2024 10:57, Greg Kroah-Hartman wrote:
> On Tue, Apr 30, 2024 at 10:50:52AM +0200, Dirk Behme wrote:
>> On 30.04.2024 10:41, Greg Kroah-Hartman wrote:
>>> On Tue, Apr 30, 2024 at 10:23:36AM +0200, Dirk Behme wrote:
>>>> Hi Greg,
>>>>
>>>> On 30.04.2024 09:20, 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...)
>>>>>
>>>>>> 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?
>>>>>
>>>>>> ...
>>>>>> if (dev->driver)
>>>>>> // If dev->driver is NULLed from really_probe() from here on,
>>>>>> // after above check, the system crashes
>>>>>> add_uevent_var(env, "DRIVER=%s", dev->driver->name);
>>>>>>
>>>>>> dev_driver_string() can't be used here because we want NULL and not
>>>>>> anything else in the non-init case.
>>>>>>
>>>>>> Similar cases are reported by syzkaller in
>>>>>>
>>>>>> https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
>>>>>>
>>>>>> But these are regarding the *initialization* of dev->driver
>>>>>>
>>>>>> dev->driver = drv;
>>>>>>
>>>>>> As this switches dev->driver to non-NULL these reports can be considered
>>>>>> to be false-positives (which should be "fixed" by this commit, as well,
>>>>>> though).
>>>>>>
>>>>>> Fixes: 239378f16aa1 ("Driver core: add uevent vars for devices of a class")
>>>>>> Cc: syzbot+ffa8143439596313a85a@syzkaller.appspotmail.com
>>>>>> Reviewed-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>>>>>> Tested-by: Eugeniu Rosca <eugeniu.rosca@bosch.com>
>>>>>> Signed-off-by: Dirk Behme <dirk.behme@de.bosch.com>
>>>>>> ---
>>>>>> drivers/base/core.c | 9 +++++++--
>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>>>>>> index 5f4e03336e68..99ead727c08f 100644
>>>>>> --- a/drivers/base/core.c
>>>>>> +++ b/drivers/base/core.c
>>>>>> @@ -2639,6 +2639,7 @@ static const char *dev_uevent_name(const struct kobject *kobj)
>>>>>> static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>>>>>> {
>>>>>> const struct device *dev = kobj_to_dev(kobj);
>>>>>> + struct device_driver *drv;
>>>>>> int retval = 0;
>>>>>> /* add device node properties if present */
>>>>>> @@ -2667,8 +2668,12 @@ static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
>>>>>> if (dev->type && dev->type->name)
>>>>>> add_uevent_var(env, "DEVTYPE=%s", dev->type->name);
>>>>>> - 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.
>>>>>
>>>>> So how is this actually solving anything?
>>>>
>>>>
>>>> Looking at dev_driver_string() I hoped that it just reads *once*. I.e. we
>>>> don't care if we read NULL or any valid pointer, as long as this pointer
>>>> read is done only once ("atomically"?). If READ_ONCE() doesn't do that, I
>>>> agree, it's not the (race) fix we are looking for.
>>>
>>> Yes, what if you read it, and then it is unloaded from the system before
>>> you attempt to access drv->name? not good.
>>>
>>>> Initially, I was thinking about anything like [1] below. I.e. adding a mutex
>>>> lock. But not sure if that is better (acceptable?).
>>>
>>> a proper lock is the only way to correctly solve this.
>>
>>
>> Would using device_lock()/unlock() for locking like done below [1]
>> acceptable?
>
> Why not test it out and see! :)
We tested
diff --git a/drivers/base/core.c b/drivers/base/core.c
index b93f3c5716ae..e2a1dd015074 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2723,8 +2723,11 @@ static ssize_t uevent_show(struct device *dev,
struct device_attribute *attr,
if (!env)
return -ENOMEM;
+ /* Synchronize with really_probe() */
+ device_lock(dev);
/* let the kset specific function add its keys */
retval = kset->uevent_ops->uevent(&dev->kobj, env);
+ device_unlock(dev);
if (retval)
goto out;
and it seems it fixes the issue at least for us in our tests.
It looks like really_probe() does run with device_lock() taken, already.
So we don't need to care about that.
And depending on the call stack dev_uevent() itself is used with
device_lock() taken sometimes, already, too. What means that it should
be safe to call the whole function under that lock (but can't place the
lock there).
So this patch goes one level up in the call stack of [1]:
dev_uevent+0x235/0x380 drivers/base/core.c:2670
uevent_show+0x10c/0x1f0 drivers/base/core.c:2742 <== lock here
dev_attr_show+0x3a/0xa0 drivers/base/core.c:2445
sysfs_kf_seq_show+0x17c/0x250 fs/sysfs/file.c:59
kernfs_seq_show+0x7c/0x90 fs/kernfs/file.c:205
...
However, we can't prove that uevent_show() is never called with
device_lock() held, already. And that uevent_ops->uevent() never calls
anything what might break with device_lock() taken.
Best regards
Dirk
[1] https://syzkaller.appspot.com/bug?extid=ffa8143439596313a85a
P.S.: It looks like this issue was discussed back in 2015 already ;)
https://lore.kernel.org/lkml/1421259054-2574-1-git-send-email-a.sangwan@samsung.com/
© 2016 - 2025 Red Hat, Inc.