Skip to contentSkip to conversation

usb: core: don't set drvdata to NULL in usb_unbind_interface()

PATCH New Danilo Krummrich 2 participants 2 messages Oct 10

Subsystems (2)
lore
Share & tools
lore

Danilo Krummrich

author maintainer
#0 lore raw

PATCH v2 1/1 usb: core: don't set drvdata to NULL in usb_unbind_interface()

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).

The same applies to the unwind path of usb_probe_interface(), which also calls 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.

Thus, drop usb_set_intfdata(intf, NULL) from usb_probe_interface() and 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.

Fixes a995fe1a3aa7 ("rust: driver: drop device private data post unbind")

---
v2: Also consider the unwind path of usb_probe_interface().

---
 drivers/usb/core/driver.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/usb/core/driver.c +1 -2
index 7f33fe5ba03b..9c5a10dc0542 100644
@@ -410,7 +410,6 @@ static int usb_probe_interface(struct device *dev)
 	return error;
 
  err:
-	usb_set_intfdata(intf, NULL);
 	intf->needs_remote_wakeup = 0;
 	intf->condition = USB_INTERFACE_UNBOUND;
 
@@ -497,7 +496,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 +642,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: 3857c2fe5449541e24afc5efdb0f81a8a8f9a3a0

Signature · 1 line
2.56.0
#1 lore raw
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 15 lines

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;
Quoted 6 lines
[ ... ]
Fixes a995fe1a3aa7 ("rust: driver: drop device private data post unbind")

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1