Re: 答复: 答复: [PATCH] xhci: sideband: check vdev liveness before removing endpoints on unregister

[Selvarasu Ganesan <selvarasu.g@samsung.com>]

On 9/11/2026 12:59 PM, 胡连勤 wrote:
> Hi Selva,
>
>>>    drivers/usb/host/xhci-mem.c      |  8 ++++++++
>>>    drivers/usb/host/xhci-sideband.c | 25 ++++++++++++++++++-------
>>>    2 files changed, 26 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>> index af8d4b74c4ba..afdcfb38b35f 100644
>>> --- a/drivers/usb/host/xhci-mem.c
>>> +++ b/drivers/usb/host/xhci-mem.c
>>> @@ -922,6 +922,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>>>    		dev->rhub_port->slot_id = 0;
>>>    	if (xhci->devs[slot_id] == dev)
>>>    		xhci->devs[slot_id] = NULL;
>>> +
>>> +	if (dev->sideband) {
>>> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
>>> +			  slot_id);
>>> +		dev->sideband->vdev = NULL;
>> Thanks for your updated patch.
>>
>> Dont forget to add #include <linux/usb/xhci-sideband.h> in this
>> xhci-mem.c file otherwise getting below error,
>>
>> drivers/usb/host/xhci-mem.c:928:30: error: invalid use of undefined type
>> ‘struct xhci_sideband’
>>       928 |                 dev->sideband->vdev = NULL;
>>
> Include the corresponding header file
> #include <linux/dmapool.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>   
>
>>> +		dev->sideband = NULL;
>>> +	}
>>> +
>>>    	kfree(dev);
>>>    }
>>>
>>> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
>>> index a5deeee4d5dc..6312c9e3af65 100644
>>> --- a/drivers/usb/host/xhci-sideband.c
>>> +++ b/drivers/usb/host/xhci-sideband.c
>>> @@ -472,12 +472,22 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>>>
>>>    	scoped_guard(mutex, &sb->mutex) {
>>>    		vdev = sb->vdev;
>>> -		if (!vdev)
>>> -			return;
>>> -
>>> -		for (i = 0; i < EP_CTX_PER_DEV; i++)
>>> -			if (sb->eps[i])
>>> -				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
>>> +		/*
>>> +		 * If vdev is NULL, xhci_free_virt_device() has already
>>> +		 * cleared sb->vdev and freed vdev (e.g. on
>>> +		 * COMP_USB_TRANSACTION_ERROR during address device
>>> +		 * recovery). Skip endpoint cleanup as the xHC has already
>>> +		 * disabled the slot.
>>> +		 *
>>> +		 * The interrupter and sideband instance are host-level
>>> +		 * resources independent of vdev, so still remove and free
>>> +		 * them to avoid leaks.
>>> +		 */
>>> +		if (vdev) {
>>> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
>>> +				if (sb->eps[i])
>>> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
>> I have one query on  skip endpoint cleanup due to  vdev is NULL, in this
>> case the pointers in sb->eps are not cleared. Since these pointers point
>> into the virtual device eps, any subsequent call to sideband API
>> functions like xhci_sideband_get_endpoint_buffer() that dereference
>> sb->eps could result in a use after free if the virtual device has been
>> freed. Is it possible?
> Yes, you're absolutely right. If sb->eps[] is not cleared when vdev
> is NULL, subsequent calls to xhci_sideband_get_endpoint_buffer() or
> similar API functions would dereference dangling pointers into the
> freed vdev, resulting in use-after-free.
>
> I added the else branch to clear sb->eps[]:
>
> 	} else {
> 		for (i = 0; i < EP_CTX_PER_DEV; i++)
> 			sb->eps[i] = NULL;
> 	}
>
>
> The complete code modification is as follows:
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index af8d4b74c4ba..448d28aaff3e 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -15,6 +15,7 @@
>   #include <linux/dmapool.h>
>   #include <linux/dma-mapping.h>
>   #include <linux/bitfield.h>
> +#include <linux/usb/xhci-sideband.h>
>   
>   #include "xhci.h"
>   #include "xhci-trace.h"
> @@ -922,6 +923,14 @@ void xhci_free_virt_device(struct xhci_hcd *xhci, struct xhci_virt_device *dev,
>   		dev->rhub_port->slot_id = 0;
>   	if (xhci->devs[slot_id] == dev)
>   		xhci->devs[slot_id] = NULL;
> +
> +	if (dev->sideband) {
> +		xhci_dbg(xhci, "vdev for slot %d has sideband still set at free, clearing dangling pointer\n",
> +			 slot_id);
> +		dev->sideband->vdev = NULL;
> +		dev->sideband = NULL;
> +	}
> +
>   	kfree(dev);
>   }
>   
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index a5deeee4d5dc..f979ce517163 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -472,12 +472,25 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	scoped_guard(mutex, &sb->mutex) {
>   		vdev = sb->vdev;
> -		if (!vdev)
> -			return;
> -
> -		for (i = 0; i < EP_CTX_PER_DEV; i++)
> -			if (sb->eps[i])
> -				__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		/*
> +		 * If vdev is NULL, xhci_free_virt_device() has already
> +		 * cleared sb->vdev and freed vdev (e.g. on
> +		 * COMP_USB_TRANSACTION_ERROR during address device
> +		 * recovery). Skip endpoint cleanup as the xHC has already
> +		 * disabled the slot.
> +		 *
> +		 * The interrupter and sideband instance are host-level
> +		 * resources independent of vdev, so still remove and free
> +		 * them to avoid leaks.
> +		 */
> +		if (vdev) {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				if (sb->eps[i])
> +					__xhci_sideband_remove_endpoint(sb, sb->eps[i]);
> +		} else {
> +			for (i = 0; i < EP_CTX_PER_DEV; i++)
> +				sb->eps[i] = NULL;
> +		}
>   
>   		__xhci_sideband_remove_interrupter(sb);
>   
> @@ -486,7 +499,8 @@ xhci_sideband_unregister(struct xhci_sideband *sb)
>   
>   	spin_lock_irq(&xhci->lock);
>   	sb->xhci = NULL;
> -	vdev->sideband = NULL;
> +	if (vdev)
> +		vdev->sideband = NULL;
>   	spin_unlock_irq(&xhci->lock);
>   
>   	kfree(sb);

Looks good to me.

Thanks,
Selva


>
> Thanks
> Lianqin