Re: [PATCH v3 3/3] driver core: fix potential NULL pointer dereference in dev_uevent()

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Mon, 10 Mar 2025 22:24:16 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> 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.
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

BTW, since other drivers still assigns dev->driver directly instead
of using device_set_driver(). Would we need another patch to replace
those?

Thank you,

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> v3: addressed Rafael's feedback and split device_set_driver() helper
> into a separate patch.
>  
> v2: addressed Rafael's feedback by introducing device_set_driver()
> helper that does WRITE_ONCE() to prevent tearing. 
> 
> 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 | 13 ++++++++++++-
>  drivers/base/bus.c  |  2 +-
>  drivers/base/core.c | 33 +++++++++++++++++++++++++++++++--
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index eb203cf8370b..123031a757d9 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 {
> @@ -182,8 +183,18 @@ 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"
> -	dev->driver = (struct device_driver *)drv;
> +	WRITE_ONCE(dev->driver, (struct device_driver *)drv);
>  }
>  
>  int devres_release_all(struct device *dev);
> diff --git a/drivers/base/bus.c b/drivers/base/bus.c
> index 5ea3b03af9ba..5e75e1bce551 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 b000ee61c149..cbc0099d8ef2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -2624,6 +2624,35 @@ 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 the device from a driver,
> + * we need to be careful. Binding is generally safe, at worst we miss the
> + * fact that the 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 pointer is 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);
> @@ -2655,8 +2684,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);
> -- 
> 2.49.0.rc0.332.g42c0ae87b1-goog
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>