Re: [PATCH 05/17] USB: Lock registered interfaces during manual driver binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] USB: unlocked dev->driver and state updates in
  usb_driver_claim_interface()
- [High] usb: core: unreleased pre-claim in usb_set_configuration() when
  unbound
- [High] usb: core: skipped refused-claim cleanup in
  usb_set_configuration()
- [High] usb: incomplete unbind of multi-interface drivers in
  usb_driver_release_refused_claim()
--

commit f8b8b26e3d94c65587df2d54f58d12342c54f09a
Author: Leon Romanovsky <leon@kernel.org>

USB: Lock registered interfaces during manual driver binding

This commit acquires the target USB interface device lock around
device_bind_driver() in usb_driver_claim_interface(), updates
btusb_mtk_claim_iso_intf() to acquire the USB device lock instead of the
interface lock, and adds usb_driver_release_refused_claim() to clean up
pre-claimed interfaces when binding is refused during usb_set_configuration().

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index d3a61ebc6afb0..262502642ca98 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -584,8 +584,11 @@ int usb_driver_claim_interface(struct usb_driver *driver,
>  	/* if interface was already added, bind now; else let
>  	 * the future device_add() bind it, bypassing probe()
>  	 */
> -	if (device_is_registered(dev))
> +	if (device_is_registered(dev)) {
> +		device_lock(dev);
>  		retval = device_bind_driver(dev);
> +		device_unlock(dev);
> +	}
>  
>  	if (retval) {
>  		dev->driver = NULL;

[Severity: High]
When usb_driver_claim_interface() is called on an already-registered
interface, the caller holds the parent USB device lock (udev->dev.mutex)
rather than the target interface lock (dev->mutex). Here device_lock(dev)
only wraps device_bind_driver(dev), while dev->driver, driver_data,
iface->needs_binding, and iface->condition are still written before
device_lock(dev) is acquired and rolled back after device_unlock(dev) is
released:

drivers/usb/core/driver.c:usb_driver_claim_interface() {
	...
	dev->driver = &driver->driver;
	usb_set_intfdata(iface, data);
	iface->needs_binding = 0;

	iface->condition = USB_INTERFACE_BOUND;
	...
	if (device_is_registered(dev)) {
		device_lock(dev);
		retval = device_bind_driver(dev);
		device_unlock(dev);
	}

	if (retval) {
		dev->driver = NULL;
		usb_set_intfdata(iface, NULL);
		iface->needs_remote_wakeup = 0;
		iface->condition = USB_INTERFACE_UNBOUND;
	...
}

Because several driver-core and USB sysfs paths synchronize access to a
registered interface using only device_lock(dev) without holding the parent
USB device lock, can those unlocked state mutations still race?

For example, if device_bind_driver(dev) fails and drops device_unlock(dev),
drivers/usb/core/sysfs.c:supports_autosuspend_show() can acquire
device_lock_interruptible(dev) before dev->driver is cleared:

drivers/usb/core/sysfs.c:supports_autosuspend_show() {
	s = device_lock_interruptible(dev);
	if (s < 0)
		return -EINTR;
	/* Devices will be autosuspended even when an interface isn't claimed */
	s = (!dev->driver || to_usb_driver(dev->driver)->supports_autosuspend);
	device_unlock(dev);
}

If supports_autosuspend_show() sees dev->driver != NULL in the first check
while usb_driver_claim_interface() concurrently writes dev->driver = NULL,
to_usb_driver(dev->driver)->supports_autosuspend will dereference NULL.

Similarly, under only device_lock(dev), device_trust_set_policy() in
drivers/base/trust.c can observe the transient non-NULL dev->driver before
device_bind_driver() or before the error rollback and fail with -EBUSY,
drivers/base/dd.c:__driver_attach() can observe the transient dev->driver and
skip probing, and drivers/usb/core/message.c:usb_authorize_interface() can
write intf->authorized concurrently with the unlocked writes to
iface->needs_binding and iface->needs_remote_wakeup in the same bitfield
word.

Should device_lock(dev) be held across the entire state transition and error
rollback in usb_driver_claim_interface()?

> @@ -649,6 +652,26 @@ void usb_driver_release_interface(struct usb_driver *driver,
>  }
>  EXPORT_SYMBOL_GPL(usb_driver_release_interface);
>  
> +/*
> + * The driver core refused to bind an interface that a sibling claimed before
> + * it was registered, and dropped the claim. Let the claimant release the
> + * interface instead of operating it with a bound interface but no driver.
> + */
> +void usb_driver_release_refused_claim(struct usb_interface *iface,
> +				      struct device_driver *claimant)
> +{
> +	struct device *dev = &iface->dev;
> +
> +	device_lock(dev);
> +	if (!dev->driver && iface->condition == USB_INTERFACE_BOUND) {
> +		dev->driver = claimant;
> +		usb_unbind_interface(dev);
> +		dev_set_drvdata(dev, NULL);
> +		dev->driver = NULL;
> +	}
> +	device_unlock(dev);
> +}

[Severity: High]
When a driver probes interface 0 during usb_set_configuration() and claims an
unregistered secondary interface via usb_driver_claim_interface(), calling
usb_unbind_interface() only on the refused secondary interface here invokes
driver->disconnect(iface) on that secondary interface and then re-enables its
endpoints via usb_enable_interface(udev, intf, false).

However, many multi-interface USB drivers treat disconnect() on a secondary
interface as a no-op and only tear down state when disconnect() is called on
the primary control interface. For example, in sound/usb/card.c:

sound/usb/card.c:usb_audio_disconnect() {
	struct snd_usb_audio *chip = usb_get_intfdata(intf);
	struct snd_card *card;

	if (chip == USB_AUDIO_IFACE_UNUSED)
		return;
	...
}

And in drivers/media/usb/uvc/uvc_driver.c:

drivers/media/usb/uvc/uvc_driver.c:uvc_disconnect() {
	struct uvc_device *dev = usb_get_intfdata(intf);

	usb_set_intfdata(intf, NULL);

	if (intf->cur_altsetting->desc.bInterfaceSubClass ==
	    UVC_SC_VIDEOSTREAMING)
		return;
	...
}

Drivers such as ims_pcu_disconnect(), ati_remote2_disconnect(),
line6_disconnect(), and uea_disconnect() similarly return early when
disconnect() is called on a secondary interface.

If device_bind_driver() refuses the secondary interface during device_add(),
won't unbinding only iface leave the claimant bound to the primary interface
and actively using the unbound secondary interface (whose endpoints were just
re-enabled by usb_unbind_interface() and whose dev->driver is now NULL)?

> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index a738d03c23eac..d35a7710c8ef4 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -2264,6 +2264,7 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  	 */
>  	for (i = 0; i < nintf; ++i) {
>  		struct usb_interface *intf = cp->interface[i];
> +		struct device_driver *claimant;
>  
>  		if (intf->dev.of_node &&
>  		    !of_device_is_available(intf->dev.of_node)) {
> @@ -2277,7 +2278,14 @@ int usb_set_configuration(struct usb_device *dev, int configuration)
>  			dev_name(&intf->dev), configuration,
>  			intf->cur_altsetting->desc.bInterfaceNumber);
>  		device_enable_async_suspend(&intf->dev);
> +		claimant = intf->dev.driver;
> +		if (claimant && !try_module_get(claimant->owner))
> +			claimant = NULL;
>  		ret = device_add(&intf->dev);
> +		if (!ret && claimant && !intf->dev.driver)
> +			usb_driver_release_refused_claim(intf, claimant);
> +		if (claimant)
> +			module_put(claimant->owner);

[Severity: High]
What happens if a pre-claimed interface is passed to device_add(&intf->dev),
but device_bind_driver(&intf->dev) never completes for reasons other than
__device_attach() clearing intf->dev.driver on a0 return?

First, if /sys/bus/usb/drivers_autoprobe is set to 0 before
bus_probe_device() -> device_initial_probe() runs inside device_add(),
device_initial_probe() skips __device_attach(). In that case, device_add()
returns 0 with intf->dev.driver still set to claimant, even though
device_bind_driver() and driver_bound() were never called
(!device_is_bound(&intf->dev)). Because intf->dev.driver is non-NULL,
usb_driver_release_refused_claim() is not called, the claimant continues
operating the interface without going through device_bind_driver() admission,
and later unbinding the interface via usb_driver_release_interface() or
device_del() -> __device_release_driver() calls
klist_remove(&dev->p->knode_driver) on an unattached knode, dereferencing
NULL in klist_put().

Second, if device_add(&intf->dev) fails with a negative ret (for example, at
DPMError in drivers/base/core.c:device_add(), which calls
device_set_driver(dev, NULL), or earlier before DPMError), the !ret check
skips usb_driver_release_refused_claim(). That leaves cp->interface[i] in
dev->actconfig with intf->condition == USB_INTERFACE_BOUND and
intf->dev.driver == NULL (or an unbound driver), so usb_autosuspend_device()
at the end of usb_set_configuration() -> usb_suspend_both() ->
usb_suspend_interface() sees intf->condition != USB_INTERFACE_UNBOUND and
dereferences NULL at to_usb_driver(intf->dev.driver)->suspend:

drivers/usb/core/driver.c:usb_suspend_interface() {
	if (udev->state == USB_STATE_NOTATTACHED ||
			intf->condition == USB_INTERFACE_UNBOUND)
		goto done;
	driver = to_usb_driver(intf->dev.driver);

	/* at this time we know the driver supports suspend */
	status = driver->suspend(intf, msg);
}

Should usb_set_configuration() and usb_driver_release_refused_claim() check
whether the pre-claimed interface actually finished binding via
!device_is_bound(&intf->dev) regardless of whether ret is non-zero or
intf->dev.driver is still set?

[Severity: High]
In addition, is it safe to set claimant = NULL when
try_module_get(claimant->owner) returns false here?

The driver core does not hold a module reference on drv->owner when an
earlier sibling interface probes a modular USB driver during
usb_set_configuration(). If a concurrent rmmod (delete_module) begins on
claimant->owner while usb_set_configuration() holds device_lock(&dev->dev),
try_stop_module() transitions the module to MODULE_STATE_GOING and the
module's exit path blocks in usb_deregister() -> driver_unregister() ->
driver_detach() waiting to acquire the parent USB device lock
device_lock(&dev->dev).

Because MODULE_STATE_GOING is already set, try_module_get(claimant->owner)
returns false and sets claimant = NULL even though claimant cannot disappear
while device_lock(&dev->dev) is held. If device_add(&intf->dev) then fails to
bind the pre-claimed interface and clears intf->dev.driver = NULL,
usb_driver_release_refused_claim() is skipped because claimant is NULL.

Neither driver_detach() (since intf was never added to
claimant->p->klist_devices) nor a subsequent usb_driver_release_interface()
call from the sibling interface's disconnect callback (which returns early
when !dev->driver) will reset intf->condition, leaving intf->condition ==
USB_INTERFACE_BOUND with intf->dev.driver == NULL and causing
usb_suspend_interface() to dereference a NULL driver pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-device-trust-core-v1-0-1e536825b4eb@nvidia.com?part=5