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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

On Thu, Feb 20, 2025 at 11:59:02AM +0100, Rafael J. Wysocki wrote:
> On Thu, Feb 20, 2025 at 7:47 AM 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.
> >
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.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.

Yes, indeed, thank you Rafael. I forgot that it is no longer guaranteed
that we won't have tearing when writing native word sizes.

Thanks.

-- 
Dmitry