Race condition when accessing authorized_default for a root hub

Race condition when accessing authorized_default for a root hub

[Thiébaud Weksteen]

Hi,

I believe there is a race condition for userland in hcd.c where the
kernel sends a uevent before the underlying sysfs is fully populated:

(drivers/usb/core/hcd.c)
          /* starting here, usbcore will pay attention to this root hub */
          retval = register_root_hub(hcd);
          if (retval != 0)
                  goto err_register_root_hub;
          [...]
          retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
          if (retval < 0) {
                  printk(KERN_ERR "Cannot register USB bus sysfs
attributes: %d\n",
                         retval);
                  goto error_create_attr_group;
          }

Here, register_root_hub will call kobject_uevent(&dev->kobj, KOBJ_ADD)
(via usb_new_device and device_add). This was found in userland (see
https://github.com/USBGuard/usbguard/issues/321).

The fix is not as simple as swapping both blocks (that is, calling
sysfs_create_group before register_root_hub) as device_add is
responsible for calling kobject_add and kobject_uevent.

This can potentially be solved by adding the sysfs_create_group call
to the bus_notifier list (blocking). Thoughts?

Thanks,
Thiebaud

Re: Race condition when accessing authorized_default for a root hub

[Greg Kroah-Hartman]

On Mon, Aug 05, 2019 at 12:29:27PM +0200, Thiébaud Weksteen wrote:
> Hi,
> 
> I believe there is a race condition for userland in hcd.c where the
> kernel sends a uevent before the underlying sysfs is fully populated:
> 
> (drivers/usb/core/hcd.c)
>           /* starting here, usbcore will pay attention to this root hub */
>           retval = register_root_hub(hcd);
>           if (retval != 0)
>                   goto err_register_root_hub;
>           [...]
>           retval = sysfs_create_group(&rhdev->dev.kobj, &usb_bus_attr_group);
>           if (retval < 0) {
>                   printk(KERN_ERR "Cannot register USB bus sysfs
> attributes: %d\n",
>                          retval);
>                   goto error_create_attr_group;
>           }
> 
> Here, register_root_hub will call kobject_uevent(&dev->kobj, KOBJ_ADD)
> (via usb_new_device and device_add). This was found in userland (see
> https://github.com/USBGuard/usbguard/issues/321).
> 
> The fix is not as simple as swapping both blocks (that is, calling
> sysfs_create_group before register_root_hub) as device_add is
> responsible for calling kobject_add and kobject_uevent.
> 
> This can potentially be solved by adding the sysfs_create_group call
> to the bus_notifier list (blocking). Thoughts?

Yes, this type of race condition is _ALL_ over the kernel tree.  I am
working on solving these with the addition of the dev_groups pointer to
struct driver that went into linux-next last week.  I have a series of
patches I'm working on for all USB drivers to take advantage of this and
will post them later this week when they are done.

Ideally no driver should ever call sysfs_create_group(s) on their own,
their "parent" code should be the one doing it (either in the driver
core, or in the bus specific code.)  But, this one is a bit tricker as
root hubs are a bit different.  I'll add this to my list to fix up,
unless you want to send a patch earlier?

And as I said above, this is a well-known, and very old issue.  If you
are relying on sysfs attributes to always be present when KOBJ_ADD is
called, you _will_ race with the kernel at the moment.  Hopefully these
can all be fixed up so that next year you will not have to worry about
this :)

thanks,

greg k-h