Re: [PATCH v2 11/11] xhci: Prevent invalid vdev dereference during sideband unregister

[Mathias Nyman <mathias.nyman@linux.intel.com>]

On 10/9/26 18:30, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] xhci: TOCTOU race and Use-After-Free in sideband registration
> --
> 
> commit 7085c7c59881c747b14aeec07262203621aaa8b5
> Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> xhci: Prevent invalid vdev dereference during sideband unregister
> 
> This commit prevents the reallocation of the xhci virtual device in the setup
> device error path if a sideband is registered. It aims to fix a kernel oops
> caused by dereferencing a stale vdev pointer during USB audio disconnect.
> 
> Closes: https://lore.kernel.org/linux-usb/TYUPR06MB6217000B59003EDF233D7246D2B22@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4435,10 +4435,13 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct usb_device *udev,
>>   		dev_warn(&udev->dev, "Device not responding to setup %s.\n", act);
>>   
>>   		mutex_unlock(&xhci->mutex);
>> -		ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
>> -		if (!ret) {
>> -			if (xhci_alloc_dev(hcd, udev) == 1)
>> -				xhci_setup_addressable_virt_dev(xhci, udev);
>> +
>> +		if (!virt_dev->sideband) {
>> +			ret = xhci_disable_and_free_slot(xhci, udev->slot_id);
> 
> [Severity: High]
> Can this lockless check introduce a Time-Of-Check to Time-Of-Use (TOCTOU)
> race condition leading to a use-after-free?
> 
> If an asynchronous caller like snd_usb_rediscover_devices() calls
> xhci_sideband_register() concurrently, it acquires xhci->lock, reads
> xhci->devs[slot_id], and attaches the sideband.
> 
> If this lockless check evaluates to true just before the sideband is
> attached, we proceed to xhci_disable_and_free_slot() without the lock:
> 
> xhci_setup_device()
>      if (!virt_dev->sideband) {
>          xhci_disable_and_free_slot()
>              xhci_free_virt_device()
>                  xhci->devs[slot_id] = NULL;
>                  kfree(dev);
> 
> Because xhci_free_virt_device() does not hold xhci->lock, it can free the
> device immediately after xhci_sideband_register() attaches the sideband,
> leaving the sideband with a dangling pointer to a freed vdev.

Not an issue, or extremely unlikely.

This xhci_setup_device() codepath is called during usb device (re-)enumeration.
In this case problematic case is after a failed usb port resume calling reset-resume.

This means that the suspended audio class interface driver would have to register
audio sideband before the audio class interface itself resumed, and do this
while the parent port is mid resume.

Mathias