On Thu, Mar 6, 2025 at 7:51 AM Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
>
> If userspace reads "uevent" device attribute at the same time as another
> threads unbinds the device from its driver, change to dev->driver from a
> valid pointer to NULL may result in crash. Fix this by using READ_ONCE()
> when fetching the pointer, and take bus' drivers klist lock to make sure
> driver instance will not disappear while we access it.
>
> Use WRITE_ONCE() when setting the driver pointer to ensure there is no
> tearing.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> ---
>
> v2: addressed Rafael's feedback by introducing device_set_driver()
> helper that does WRITE_ONCE() to prevent tearing.
So the code changes look OK to me now, but I would introduce
device_set_driver() in a separate preliminary patch, to start with as
a simple assignment.
Then, in the fix patch proper, I'd replace the assignment in
device_set_driver() with WRITE_ONCE().
That would allow the fix itself to be distinguished from the
tangentially related changes depended on by it.
> I added Cc: stable however I do not think we need to worry too much
> about backporting it to [very] old kernels: the race window is very
> small, and in real life we do not unbind devices that often.
>
> I believe there are more questionable places where we read dev->driver
> pointer, those need to be adjusted separately.
>
> drivers/base/base.h | 18 ++++++++++++++++++
> drivers/base/bus.c | 2 +-
> drivers/base/core.c | 34 +++++++++++++++++++++++++++++++---
> drivers/base/dd.c | 7 +++----
> 4 files changed, 53 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 8cf04a557bdb..ed2d7ccc7354 100644
> --- a/drivers/base/base.h
> +++ b/drivers/base/base.h
> @@ -73,6 +73,7 @@ static inline void subsys_put(struct subsys_private *sp)
> kset_put(&sp->subsys);
> }
>
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus);
> struct subsys_private *class_to_subsys(const struct class *class);
>
> struct driver_private {
> @@ -179,6 +180,23 @@ int driver_add_groups(const struct device_driver *drv, const struct attribute_gr
> void driver_remove_groups(const struct device_driver *drv, const struct attribute_group **groups);
> void device_driver_detach(struct device *dev);
>
> +static inline void device_set_driver(struct device *dev, const struct device_driver *drv)
> +{
> +
> + /*
> + * Majority (all?) read accesses to dev->driver happens either
> + * while holding device lock or in bus/driver code that is only
> + * invoked when the device is bound to a driver and there is no
> + * concern of the pointer being changed while it is being read.
> + * However when reading device's uevent file we read driver pointer
> + * without taking device lock (so we do not block there for
> + * arbitrary amount of time). We use WRITE_ONCE() here to prevent
> + * tearing so that READ_ONCE() can safely be used in uevent code.
> + */
> + // FIXME - this cast should not be needed "soon"
> + WRITE_ONCE(dev->driver, (struct device_driver *)drv);
> +}
> +
> int devres_release_all(struct device *dev);
> void device_block_probing(void);
> void device_unblock_probing(void);
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 6b9e65a42cd2..c8c7e0804024 100644
> --- a/drivers/base/bus.c
> +++ b/drivers/base/bus.c
> @@ -57,7 +57,7 @@ static int __must_check bus_rescan_devices_helper(struct device *dev,
> * NULL. A call to subsys_put() must be done when finished with the pointer in
> * order for it to be properly freed.
> */
> -static struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> +struct subsys_private *bus_to_subsys(const struct bus_type *bus)
> {
> struct subsys_private *sp = NULL;
> struct kobject *kobj;
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 9f4d4868e3b4..27fe69d06765 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2623,6 +2623,34 @@ static const char *dev_uevent_name(const struct kobject *kobj)
> return NULL;
> }
>
> +/*
> + * Try filling "DRIVER=<name>" uevent variable for a device. Because this
> + * function may race with binding and unbinding device from a driver we need to
> + * be careful. Binding is generally safe, at worst we miss the fact that device
> + * is already bound to a driver (but the driver information that is delivered
> + * through uevents is best-effort, it may become obsolete as soon as it is
> + * generated anyways). Unbinding is more risky as driver transitioning to NULL,
> + * so READ_ONCE() should be used to make sure we are dealing with the same
> + * pointer, and to ensure that driver structure is not going to disappear from
> + * under us we take bus' drivers klist lock. The assumption that only registered
> + * driver can be bound to a device, and to unregister a driver bus code will
> + * take the same lock.
> + */
> +static void dev_driver_uevent(const struct device *dev, struct kobj_uevent_env *env)
> +{
> + struct subsys_private *sp = bus_to_subsys(dev->bus);
> +
> + if (sp) {
> + scoped_guard(spinlock, &sp->klist_drivers.k_lock) {
> + struct device_driver *drv = READ_ONCE(dev->driver);
> + if (drv)
> + add_uevent_var(env, "DRIVER=%s", drv->name);
> + }
> +
> + subsys_put(sp);
> + }
> +}
> +
> static int dev_uevent(const struct kobject *kobj, struct kobj_uevent_env *env)
> {
> const struct device *dev = kobj_to_dev(kobj);
> @@ -2654,8 +2682,8 @@ 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);
> + /* Add "DRIVER=%s" variable if the device is bound to a driver */
> + dev_driver_uevent(dev, env);
>
> /* Add common DT information about the device */
> of_device_uevent(dev, env);
> @@ -3696,7 +3724,7 @@ int device_add(struct device *dev)
> device_pm_remove(dev);
> dpm_sysfs_remove(dev);
> DPMError:
> - dev->driver = NULL;
> + device_set_driver(dev, NULL);
> bus_remove_device(dev);
> BusError:
> device_remove_attrs(dev);
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f0e4b4aba885..b526e0e0f52d 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -550,7 +550,7 @@ static void device_unbind_cleanup(struct device *dev)
> arch_teardown_dma_ops(dev);
> kfree(dev->dma_range_map);
> dev->dma_range_map = NULL;
> - dev->driver = NULL;
> + device_set_driver(dev, NULL);
> dev_set_drvdata(dev, NULL);
> if (dev->pm_domain && dev->pm_domain->dismiss)
> dev->pm_domain->dismiss(dev);
> @@ -629,8 +629,7 @@ static int really_probe(struct device *dev, const struct device_driver *drv)
> }
>
> re_probe:
> - // FIXME - this cast should not be needed "soon"
> - dev->driver = (struct device_driver *)drv;
> + device_set_driver(dev, drv);
>
> /* If using pinctrl, bind pins now before probing */
> ret = pinctrl_bind_pins(dev);
> @@ -1014,7 +1013,7 @@ static int __device_attach(struct device *dev, bool allow_async)
> if (ret == 0)
> ret = 1;
> else {
> - dev->driver = NULL;
> + device_set_driver(dev, NULL);
> ret = 0;
> }
> } else {
> --
> 2.49.0.rc0.332.g42c0ae87b1-goog
>
