Re: [PATCH] usb: core: don't set drvdata to NULL in usb_unbind_interface()

[Alan Stern]

On Fri, Oct 02, 2026 at 04:52:49PM +0200, Danilo Krummrich wrote:
> usb_unbind_interface() serves as the remove() callback of struct
> usb_driver and calls usb_set_intfdata(intf, NULL) to clear the bus
> device private data pointer.
> 
> However, the driver core code already sets the bus device private data
> pointer to NULL in device_unbind_cleanup() *after* devres_release_all(),
> which makes the call redundant.
> 
> In addition, it can create unexpected NULL pointer dereference scenarios
> when drivers use managed APIs.
> 
> 	int probe(struct usb_interface *intf,
> 		  const struct usb_device_id *id)
> 	{
> 		struct data *data;
> 		int ret;
> 
> 		data = devm_kzalloc(&intf->dev, sizeof(*data), GFP_KERNEL);
> 		if (!data)
> 			return -ENOMEM;
> 
> 		ret = devm_device_add_group(&intf->dev, &foo_attr_group);
> 		if (ret)
> 			return ret;
> 
> 		...
> 	}
> 
> 	ssize_t foo_value_show(struct device *dev, struct device_attribute *attr,
> 			       char *buf)
> 	{
> 		struct usb_interface *intf = to_usb_interface(dev);
> 		struct data *data = usb_get_intfdata(intf);
> 
> 		/* Potential NULL pointer dereference */
> 		return sysfs_emit(buf, "%u\n", data->value);
> 	}
> 
> Nothing prevents usb_unbind_interface() to race with foo_value_show()
> and set usb_set_intfdata(intf, NULL).
> 
> Besides that, the Rust driver core code manages a driver's bus device
> private data and destroys it in device_unbind_cleanup().
> 
> If usb_unbind_interface() sets the pointer to NULL prematurely, the Rust
> driver core code sees NULL, and hence skips the destructor of the bus
> device private data, which leaks all its resources.

Aren't there a few interface drivers that explicitly set the intfdata 
value to NULL in their own ->remove() routines?  If you haven't checked 
for that, you might want to.

> Thus, drop usb_set_intfdata(intf, NULL) from usb_unbind_interface() and
> move it to usb_driver_release_interface(), which manually calls the
> remove() callback of struct usb_driver, and hence can't rely on the
> driver core.
> 
> Cc: stable@xxxxxxxxxx
> Fixes: a995fe1a3aa7 ("rust: driver: drop device private data post unbind")
> Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
> ---

Makes sense,  IIRC, that line was added mostly as a convenience for 
drivers, anyway.

Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>

Alan Stern

>  drivers/usb/core/driver.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 7f33fe5ba03b..412174270fbf 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -497,7 +497,6 @@ static int usb_unbind_interface(struct device *dev)
>  	} else {
>  		intf->needs_altsetting0 = 1;
>  	}
> -	usb_set_intfdata(intf, NULL);
>  
>  	intf->condition = USB_INTERFACE_UNBOUND;
>  	intf->needs_remote_wakeup = 0;
> @@ -644,6 +643,7 @@ void usb_driver_release_interface(struct usb_driver *driver,
>  	} else {
>  		device_lock(dev);
>  		usb_unbind_interface(dev);
> +		dev_set_drvdata(dev, NULL);
>  		dev->driver = NULL;
>  		device_unlock(dev);
>  	}
> 
> base-commit: ce1e0223d8ad4211275c82a17ed6d43ab81e13d9
> -- 
> 2.56.0