Re: [PATCH v4] usb: xhci: Fix use-after-free race in xhci_stop_device()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] usb: xhci: commit message says CLEAR_FEATURE instead of SET_FEATURE
--

commit 8ca4e8a9c32024fa63955a4430862a58450a721c
Author: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

usb: xhci: Fix use-after-free race in xhci_stop_device()

This commit fixes a use-after-free race in xhci_stop_device() by checking
xhci->devs[slot_id] and virt_dev->udev under xhci->lock, and clearing
virt_dev->udev under the same lock in xhci_free_dev().

>     usb: xhci: Fix use-after-free race in xhci_stop_device()
>     
>     A KASAN slab-use-after-free read was observed in xhci_stop_device():
>     
>       BUG: KASAN: slab-use-after-free in xhci_stop_device.constprop.0.isra.0+0x395/0x430
>       drivers/usb/host/xhci-hub.c:479
>       Read of size 8 at addr ffff88804e85d2f0
>     
>       Freed by task 585700:
>        kfree+0xf8/0x390 mm/slub.c:6566
>        xhci_free_virt_device+0x3f/0x50 drivers/usb/host/xhci-mem.c:881
>        xhci_free_dev+0x286/0x350 drivers/usb/host/xhci.c:4131
>        usb_disconnect+0x6b6/0x910 drivers/usb/core/hub.c:2391
>        ...
>        usbdev_ioctl+0x6f05/0x86d0 drivers/usb/core/devio.c:2826
>     
>       Allocated by task 572283:
>        xhci_alloc_virt_device+0x171/0xb70 drivers/usb/host/xhci-mem.c:986
>        xhci_alloc_dev+0x475/0x840 drivers/usb/host/xhci.c:4271
>        usb_alloc_dev+0xe8/0xdc0 drivers/usb/core/usb.c:661
>        hub_event+0x1c82/0x4050 drivers/usb/core/hub.c:5953
>     
>     xhci_stop_device() reads xhci->devs[slot_id] without xhci->lock and
>     keeps using the pointer across xhci_alloc_command(GFP_NOIO), which may
>     sleep.  All callers drop xhci->lock before calling it.  Both sides of
>     the race can come from userspace through usbfs: a hub class
>     CLEAR_FEATURE(PORT_SUSPEND) request to the root hub reaches
>     xhci_stop_device() through xhci_hub_control(), while releasing the hub
>     driver interface through USBDEVFS_IOCTL() quiesces the hub and
>     disconnects the devices below it, ending in xhci_free_dev().  USB core
>     does not serialize those two, and xhci_free_dev() frees virt_dev under
>     xhci->lock, so the endpoint loop can dereference a freed device.
>     
>     commit 88716a93766b ("usb: xhci: Fix panic if disconnect") added a
>     NULL check for this case, but that only helps when the device was
>     already freed before the check; it does not close the window.
>     
>     Look up xhci->devs[slot_id] only after taking xhci->lock, and bail out
>     if it is NULL or if the device is already being torn down.  The
>     tracepoint reloads virt_dev->udev, so it moves under the lock and only
>     runs while the device is still attached.
>     
>     xhci_free_dev() clears virt_dev->udev without xhci->lock, and only
>     later detaches the device from xhci->devs[] in xhci_free_virt_device(),
>     also under the lock.  Take xhci->lock when clearing ->udev so that
>     xhci_stop_device() can rely on it while holding the same lock.
>     xhci_free_virt_device() clears xhci->devs[slot_id] and frees the device
>     under xhci->lock, so a device found there while holding the lock stays
>     alive until xhci_stop_device() drops the lock, after which virt_dev is
>     not used anymore.
>     
>     Fixes: be88fe4f4dda93e3 ("USB: xHCI: port power management implementation")
>     Cc: stable@vger.kernel.org
>     Co-developed-by: Bin Yu <byu@xidian.edu.cn>
>     Signed-off-by: Bin Yu <byu@xidian.edu.cn>
>     Co-developed-by: Cheng Wen <wencheng@xidian.edu.cn>
>     Signed-off-by: Cheng Wen <wencheng@xidian.edu.cn>
>     Signed-off-by: YuanZhe Liu <25031212351@stu.xidian.edu.cn>

[Severity: Low]
This isn't a bug in the code, but the commit message says that a
CLEAR_FEATURE(PORT_SUSPEND) request reaches xhci_stop_device() through
xhci_hub_control().

In drivers/usb/host/xhci-hub.c:xhci_hub_control(), ClearPortFeature with
USB_PORT_FEAT_SUSPEND resumes the port and calls xhci_ring_device():

	case ClearPortFeature:
		...
		switch (wValue) {
		case USB_PORT_FEAT_SUSPEND:
			...
			if (!port->slot_id) {
				xhci_dbg(xhci, "slot_id is zero\n");
				goto error;
			}
			xhci_ring_device(xhci, port->slot_id);
			break;

Whereas xhci_stop_device() is called under SetPortFeature with
USB_PORT_FEAT_SUSPEND (or USB_PORT_FEAT_LINK_STATE with USB_SS_PORT_LS_U3):

	case SetPortFeature:
		...
		switch (wValue) {
		case USB_PORT_FEAT_SUSPEND:
			...
			/* unlock to execute stop endpoint commands */
			spin_unlock_irqrestore(&xhci->lock, flags);
			xhci_stop_device(xhci, port->slot_id, 1);
			spin_lock_irqsave(&xhci->lock, flags);

Should the commit message say SET_FEATURE(PORT_SUSPEND) instead of
CLEAR_FEATURE(PORT_SUSPEND)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104013221.3223-1-25031212351@stu.xidian.edu.cn?part=1