Re: [PATCH v2 04/19] USB: Release interface claims refused during registration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] usb: core: unreleased interface claim when device_add() fails
  or skips probe
- [Medium] usb: core: skipped claim release when try_module_get() fails
  in usb_set_configuration()
--

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

USB: Release interface claims refused during registration

This commit tracks pre-registration interface claims across device_add() in
usb_set_configuration() and invokes usb_driver_release_refused_claim() to
unbind the interface if the driver core drops the claim during registration.

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

[Severity: Medium]
Does clearing claimant to NULL when try_module_get() fails skip releasing
the refused claim if the claimant module is concurrently unloading?

Binding a USB driver to an interface does not take a module reference on
claimant->owner.  If delete_module() races with usb_set_configuration(),
try_stop_module() sets MODULE_STATE_GOING (causing try_module_get() to fail)
before mod->exit() -> usb_deregister() -> driver_detach() blocks on
device_lock(&dev->dev) while detaching the already-bound sibling interface.

If device_add() then fails to bind intf and clears intf->dev.driver to NULL,
usb_driver_release_refused_claim() is skipped because claimant is NULL, and
the sibling's later usb_driver_release_interface() call returns early because
intf->dev.driver is already NULL.  That leaves intf registered with
intf->condition == USB_INTERFACE_BOUND and intf->dev.driver == NULL, which
causes a NULL pointer dereference in usb_suspend_interface().

>  		ret = device_add(&intf->dev);
> +		if (!ret && claimant && !intf->dev.driver)
> +			usb_driver_release_refused_claim(intf, claimant);

[Severity: Medium]
Can a pre-claimed interface also be left in USB_INTERFACE_BOUND state without
a bound driver when device_add() returns an error or skips probing?

If dpm_sysfs_add() fails in device_add(), its DPMError path calls
device_set_driver(dev, NULL) and returns a non-zero ret:

drivers/base/core.c:device_add() {
    ...
 DPMError:
	device_set_driver(dev, NULL);
	bus_remove_device(dev);
    ...
}

Because usb_set_configuration() logs the device_add() error and continues,
skipping usb_driver_release_refused_claim() when ret != 0 leaves intf in
dev->actconfig->interface[i] with intf->condition == USB_INTERFACE_BOUND and
intf->dev.driver == NULL, which crashes in usb_suspend_interface() and
prevents usb_driver_release_interface() from cleaning it up.  If device_add()
fails before DPMError, intf remains unregistered in USB_INTERFACE_BOUND
state, so usb_disable_device() also skips unbinding it on teardown.

Similarly, if bus autoprobe is disabled when device_initial_probe() runs,
device_add() returns 0 without calling device_bind_driver() or clearing
intf->dev.driver, leaving intf->dev.driver set while !device_is_bound(&intf->dev).
Checking !device_is_bound(&intf->dev) instead of !ret && !intf->dev.driver
would cover all of these cases.

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