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