[PATCH] USB: core: don't release an unbound claimed interface via the driver core

[PATCH] USB: core: don't release an unbound claimed interface via the driver core

[Nguyen Ngoc Thang]

usb_driver_claim_interface() sets dev->driver on an interface and only
calls device_bind_driver() if the interface is already registered.
Otherwise the later device_add() is expected to bind it, but with
bus drivers_autoprobe disabled bus_probe_device() skips the attach, so
the interface ends up with dev->driver set while it was never added to
the driver's klist.

usb_driver_release_interface() then sees a registered device and calls
device_release_driver(), which does klist_remove() on a node that is
not attached and dereferences a NULL klist:

  KASAN: null-ptr-deref in range [0x58-0x5f]
  RIP: klist_put+0x4d/0x1d0
   klist_remove+0x14c/0x2e0
   device_release_driver_internal+0x4fb/0x620
   usb_driver_release_interface+0x10e/0x190
   cdc_ncm_unbind+0x2ac/0x350
   usbnet_disconnect+0x1e2/0x300
   usb_unbind_interface+0x1dd/0x9e0

Decide on device_is_bound() instead of device_is_registered(): only go
through the driver core if it really bound the interface, otherwise use
the same direct unbind as for an unregistered interface.

Reported-by: syzbot+863936f50214e843ae0c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=863936f50214e843ae0c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
Root cause: cdc_ncm claims its data interface from probe of the control
interface, before the data interface is registered, so only dev->driver
is set and the bind is left to device_add().  With drivers_autoprobe=0
that bind never happens, so release goes through device_release_driver()
on an interface that was never put on the driver's klist.

Tested with the syzbot C reproducer in QEMU (KASAN, dummy_hcd) on
111dc5487a4e: the baseline oopses in klist_put() in 2 of 6 runs (the
reproducer is racy), the patched kernel in 0 of 10.  A temporary
pr_info confirmed the new path is taken in a run that would otherwise
crash.  The Fixes tag points at the start of git history, as the code
predates it.

 drivers/usb/core/driver.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index 7f33fe5ba03b..c7922875dd59 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -636,10 +636,11 @@ void usb_driver_release_interface(struct usb_driver *driver,
 		return;
 	iface->condition = USB_INTERFACE_UNBINDING;
 
-	/* Release via the driver core only if the interface
-	 * has already been registered
+	/* Release via the driver core only if the interface has been
+	 * bound by it.  A claimed interface is not bound if it was
+	 * registered with drivers_autoprobe disabled.
 	 */
-	if (device_is_registered(dev)) {
+	if (device_is_bound(dev)) {
 		device_release_driver(dev);
 	} else {
 		device_lock(dev);
-- 
2.43.0

Re: [PATCH] USB: core: don't release an unbound claimed interface via the driver core

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] usb: core: incomplete fix allows KASAN null-ptr-deref via sysfs unbind
--

commit fd7979038e7e5e8a14bb9739dc4ce0f3b1576847
Author: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

USB: core: don't release an unbound claimed interface via the driver core

usb_driver_claim_interface() sets dev->driver on an interface and only
calls device_bind_driver() if the interface is already registered.
Otherwise the later device_add() is expected to bind it, but with
bus drivers_autoprobe disabled bus_probe_device() skips the attach, so
the interface ends up with dev->driver set while it was never added to
the driver's klist.

Closes: https://syzkaller.appspot.com/bug?extid=863936f50214e843ae0c

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index 3c3bbaf89609a..20e8e61bf6713 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -636,10 +636,11 @@ void usb_driver_release_interface(struct usb_driver *driver,
>  		return;
>  	iface->condition = USB_INTERFACE_UNBINDING;
>  
> -	/* Release via the driver core only if the interface
> -	 * has already been registered
> +	/* Release via the driver core only if the interface has been
> +	 * bound by it.  A claimed interface is not bound if it was
> +	 * registered with drivers_autoprobe disabled.
>  	 */
> -	if (device_is_registered(dev)) {
> +	if (device_is_bound(dev)) {

[Severity: High]
Does this change fully resolve the issue, or does the crash still trigger
via the sysfs unbind path?

If drivers_autoprobe=0 and the CDC NCM control interface is manually bound,
usb_driver_claim_interface() sets dev->driver for the data interface without
actually binding it, leaving its klist node unattached.

This patch prevents the crash in usb_driver_release_interface() by checking
device_is_bound(), but what happens if an administrator writes the data
interface's device name to /sys/bus/usb/drivers/cdc_ncm/unbind?

The unbind_store() function in drivers/base/bus.c will call
device_driver_detach(), reaching __device_release_driver():

drivers/base/dd.c:__device_release_driver() {
        ...
        if (dev->driver) {
                ...
                klist_remove(&dev->p->knode_driver);
                ...
}

Since dev->driver is set, will this still attempt to remove the unattached
klist node and result in the same NULL pointer dereference in klist_put()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008003431.145352-1-ngocthang2710.1999@gmail.com?part=1

[PATCH v2] driver core: bind a device with a preset driver even without autoprobe

[Nguyen Ngoc Thang]

usb_driver_claim_interface() may claim an interface that is not yet
registered, in which case it only sets dev->driver and relies on the
later device_add() to bind it via __device_attach().  With the bus
drivers_autoprobe disabled, device_initial_probe() skips the attach, so
the device is registered with dev->driver set but never added to the
driver's klist.

Releasing such a device (from usb_driver_release_interface() or from
sysfs unbind) makes __device_release_driver() call klist_remove() on an
unattached node and dereference a NULL klist:

  KASAN: null-ptr-deref in range [0x58-0x5f]
  RIP: klist_put+0x4d/0x1d0
   klist_remove+0x14c/0x2e0
   device_release_driver_internal+0x4fb/0x620
   usb_driver_release_interface+0x10e/0x190
   cdc_ncm_unbind+0x2ac/0x350
   usbnet_disconnect+0x1e2/0x300
   usb_unbind_interface+0x1dd/0x9e0

Autoprobe only governs matching drivers to devices; a device that
already has a driver needs no matching, just the bind that was promised
at claim time.  Do not skip that bind.

Reported-by: syzbot+863936f50214e843ae0c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=863936f50214e843ae0c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v2: Sashiko pointed out that v1 only closed one entry point: a sysfs
unbind of the claimed-but-unbound data interface would still reach
klist_remove() on an unattached node.  Fix the state itself instead:
bind the device when a driver was preset, as claim promised.  v1 was
"USB: core: don't release an unbound claimed interface via the driver
core" (device_is_bound() in usb_driver_release_interface()), dropped.

Tested with the syzbot C reproducer in QEMU (KASAN, dummy_hcd).  The
race that leaves the interface unbound is rare, so to make it
deterministic I temporarily made device_initial_probe() behave as if
autoprobe were off whenever dev->driver is set: the unpatched logic
oopses 3/3, with this patch 0/4.  The plain patched kernel, without
the hack, shows no oops either.

 drivers/base/dd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index f6525a7ee8c5..137bf1dea333 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1149,7 +1149,8 @@ void device_initial_probe(struct device *dev)
 	if (!sp)
 		return;
 
-	if (sp->drivers_autoprobe)
+	/* a device with a preset driver must still be bound */
+	if (sp->drivers_autoprobe || dev->driver)
 		__device_attach(dev, true);
 
 	subsys_put(sp);
-- 
2.43.0

Re: [PATCH v2] driver core: bind a device with a preset driver even without autoprobe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] driver core: lockless TOCTOU race in device_initial_probe() bypasses autoprobe=0
--

commit e63fcf260c7ac274ae164a5fb03accba2fcada52
Author: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>

driver core: bind a device with a preset driver even without autoprobe

When drivers_autoprobe is disabled, device_initial_probe() skipped
calling __device_attach(). This commit ensures that devices with a
preset driver still undergo binding even if autoprobe is disabled,
preventing a NULL pointer dereference in klist_remove() on unbind.

Closes: https://syzkaller.appspot.com/bug?extid=863936f50214e843ae0c

> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index f6525a7ee8c54..137bf1dea3333 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1149,7 +1149,8 @@ void device_initial_probe(struct device *dev)
>  	if (!sp)
>  		return;
>  
> -	if (sp->drivers_autoprobe)
> +	/* a device with a preset driver must still be bound */
> +	if (sp->drivers_autoprobe || dev->driver)
>  		__device_attach(dev, true);

[Severity: Medium]
Does this lockless check of dev->driver introduce a TOCTOU race that
bypasses the autoprobe disabled state?

If sp->drivers_autoprobe is disabled, this code relies on dev->driver
being non-NULL to trigger __device_attach(). However, if dev->driver is
cleared concurrently (for instance, via a hardware disconnect or a sysfs
unbind), __device_attach() will see dev->driver as NULL under the
device lock.

When __device_attach() sees dev->driver as NULL, it takes the else branch
and executes the fallback bus_for_each_drv() probe:

drivers/base/dd.c:__device_attach() {
    ...
	} else if (dev->driver) {
    ...
	} else {
		struct device_attach_data data = {
			.dev = dev,
			.check_async = allow_async,
			.want_async = false,
		};
    ...
		ret = bus_for_each_drv(dev->bus, NULL, &data,
					__device_attach_driver);
    ...
}

Will this cause an unintended auto-probe across all drivers on the bus,
violating the autoprobe=0 configuration?

>  
>  	subsys_put(sp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008144859.25548-1-ngocthang2710.1999@gmail.com?part=1

[PATCH v3] driver core: bind a device with a preset driver even without autoprobe

[Nguyen Ngoc Thang]

usb_driver_claim_interface() may claim an interface that is not yet
registered, in which case it only sets dev->driver and relies on the
later device_add() to bind it via __device_attach().  With the bus
drivers_autoprobe disabled, device_initial_probe() skips the attach, so
the device is registered with dev->driver set but never added to the
driver's klist.

Releasing such a device (from usb_driver_release_interface() or from
sysfs unbind) makes __device_release_driver() call klist_remove() on an
unattached node and dereference a NULL klist:

  KASAN: null-ptr-deref in range [0x58-0x5f]
  RIP: klist_put+0x4d/0x1d0
   klist_remove+0x14c/0x2e0
   device_release_driver_internal+0x4fb/0x620
   usb_driver_release_interface+0x10e/0x190
   cdc_ncm_unbind+0x2ac/0x350
   usbnet_disconnect+0x1e2/0x300
   usb_unbind_interface+0x1dd/0x9e0

Autoprobe only governs matching drivers to devices; a device that
already has a driver needs no matching, just the bind that was promised
at claim time.  Do that bind under the device lock when autoprobe is
off, so that a concurrently cleared dev->driver cannot fall through to
probing every driver on the bus.

Reported-by: syzbot+863936f50214e843ae0c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=863936f50214e843ae0c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
---
v3: Sashiko noted that the lockless dev->driver check in v2 could race
with dev->driver being cleared, letting __device_attach() probe every
driver despite autoprobe being off.  Do the preset bind in a helper that
checks dev->driver under the device lock.
v2: fix the unbound state itself rather than usb_driver_release_interface()
(a sysfs unbind would still hit it).

Tested with the syzbot C reproducer in QEMU (KASAN, dummy_hcd).  The
race that leaves the interface unbound is rare, so to make it
deterministic I temporarily made device_initial_probe() behave as if
autoprobe were off whenever dev->driver is set: the unpatched logic
oopses 3/3, with this patch 0/4 (64 USB connects each).  The plain
patched kernel shows no oops either.

 drivers/base/dd.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 137bf1dea333..bb1e76bf6122 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1142,6 +1142,16 @@ int device_attach(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(device_attach);
 
+/* Bind a device whose driver was set before device_add(), no matching. */
+static void device_bind_preset(struct device *dev)
+{
+	device_lock(dev);
+	if (!dev->p->dead && dev->driver && !device_is_bound(dev) &&
+	    device_bind_driver(dev))
+		device_set_driver(dev, NULL);
+	device_unlock(dev);
+}
+
 void device_initial_probe(struct device *dev)
 {
 	struct subsys_private *sp = bus_to_subsys(dev->bus);
@@ -1149,9 +1159,10 @@ void device_initial_probe(struct device *dev)
 	if (!sp)
 		return;
 
-	/* a device with a preset driver must still be bound */
-	if (sp->drivers_autoprobe || dev->driver)
+	if (sp->drivers_autoprobe)
 		__device_attach(dev, true);
+	else
+		device_bind_preset(dev);
 
 	subsys_put(sp);
 }
-- 
2.43.0