Re: [PATCH v2 03/19] USB: Lock registered interfaces during manual driver binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] usb: core: unlocked dev->driver updates in
  usb_driver_claim_interface()
--

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

USB: Lock registered interfaces during manual driver binding

This commit locks registered USB interfaces around device_bind_driver() in
usb_driver_claim_interface() and updates btusb_mtk_claim_iso_intf() to acquire
the USB device lock instead of the target interface lock.

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index d3a61ebc6afb0..ad5d9d5fc7598 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]
Does taking device_lock(dev) only around device_bind_driver() leave
dev->driver and the interface binding state unprotected on a registered
interface?

Callers of usb_driver_claim_interface() hold the parent USB device lock, not
the target interface lock.  When device_is_registered(dev) is true,
usb_driver_claim_interface() checks dev->driver and iface->authorized and
writes dev->driver, intfdata, iface->needs_binding, and iface->condition
before acquiring device_lock(dev), and rolls them back after releasing
device_unlock(dev) if device_bind_driver() fails.

Readers that synchronize using only the interface lock, such as
supports_autosuspend_show(), can race with the unlocked rollback and
dereference a NULL dev->driver:

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

CPU0                                  CPU1
usb_driver_claim_interface()
  dev->driver = &driver->driver
  device_lock(dev)
  device_bind_driver(dev) -> fails
  device_unlock(dev)
                                      supports_autosuspend_show()
                                        device_lock_interruptible(dev)
                                        !dev->driver (false)
  dev->driver = NULL
                                        to_usb_driver(dev->driver)
                                          ->supports_autosuspend
                                          <-- NULL pointer dereference

Setting dev->driver outside device_lock(dev) also allows
device_trust_set_policy() and usb_authorize_interface() to observe a
transient non-NULL dev->driver before admission completes or after it fails,
races with __driver_attach() checking !dev->driver under device_lock(dev),
and modifies struct usb_interface bitfields (needs_binding,
needs_remote_wakeup) without serializing against iface->authorized updates.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-device-trust-core-v2-0-3b12d9a9f861@nvidia.com?part=3