Messages in this thread Patch in this message | | | Date | Tue, 30 Apr 2024 10:23:36 +0200 | Subject | Re: [PATCH] drivers: core: Make dev->driver usage safe in dev_uevent() | From | Dirk Behme <> |
| |
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; }
| |