driver core: fix potential NULL pointer dereference in dev_uevent()

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.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Link: https://lore.kernel.org/r/20250311052417.1846985-3-dmitry.torokhov@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/base/base.h b/drivers/base/base.h
index eb203cf8370bc9e8cfd35d2871cf94962a5075b5..123031a757d916d230c054a553706c34773fd317 100644 (file)
--- 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 5ea3b03af9ba6dce137c7e7454c87693ac949299..5e75e1bce5516d2d30884fb3d5fb5a976cc95cbb 100644 (file)
--- 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 b000ee61c1494e6e2000a9ea51a5f7e6848d63b6..cbc0099d8ef2464c0cd1fbd7128d669b326a1212 100644 (file)
--- 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);