linux-kernel - Re: [PATCH 2/2] driver core: fix potential NULL pointer dereference in dev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAJZ5v0geh6yAfOgo+ayvXaTTpq=ubQkM-qU22ETB6b24EOULLg@mail.gmail.com>
Date: Thu, 20 Feb 2025 11:59:02 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Danilo Krummrich <dakr@...nel.org>, linux-kernel@...r.kernel.org, 
	"Masami Hiramatsu (Google)" <mhiramat@...nel.org>, Dirk Behme <dirk.behme@...bosch.com>
Subject: Re: [PATCH 2/2] driver core: fix potential NULL pointer dereference
 in dev_uevent()

On Thu, Feb 20, 2025 at 7:47 AM Dmitry Torokhov
<dmitry.torokhov@...il.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.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> ---
>  drivers/base/base.h |  1 +
>  drivers/base/bus.c  |  2 +-
>  drivers/base/core.c | 32 ++++++++++++++++++++++++++++++--
>  3 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/base.h b/drivers/base/base.h
> index 8cf04a557bdb..91b786891209 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 {
> 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..670f77b9b378 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);

I think you need to use a matching WRITE_ONCE() on the update side
because you don't want to READ_ONCE() a partially updated memory
location.

> +                       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);
> --