linux-kernel - Re: [RFT PATCH v2] xhci: sideband: Fix race condition in sideband unregister

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2025102948-trickery-creative-417e@gregkh>
Date: Wed, 29 Oct 2025 11:14:02 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>
Cc: uttkarsh.aggarwal@....qualcomm.com, linux-kernel@...r.kernel.org,
	linux-usb@...r.kernel.org, mathias.nyman@...el.com,
	wesley.cheng@....qualcomm.com
Subject: Re: [RFT PATCH v2] xhci: sideband: Fix race condition in sideband
 unregister

On Tue, Oct 28, 2025 at 06:51:53PM +0200, Mathias Nyman wrote:
> Uttkarsh Aggarwal observed a kernel panic during sideband un-register
> and found it was caused by a race condition between sideband unregister,
> and creating sideband interrupters.
> The issue occurrs when thread T1 runs uaudio_disconnect() and released
> sb->xhci via sideband_unregister, while thread T2 simultaneously accessed
> the now-NULL sb->xhci in xhci_sideband_create_interrupter() resulting in
> a crash.
> 
> Ensure new endpoints or interrupter can't be added to a sidenband after
> xhci_sideband_unregister() cleared the existing ones, and unlocked the
> sideband mutex.
> Reorganize code so that mutex is only taken and released once in
> xhci_sideband_unregister(), and clear sb->vdev while mutex is taken.
> 
> Use mutex guards to reduce human unlock errors in code
> 
> Refuse to add endpoints or interrupter if sb->vdev is not set.
> sb->vdev is set when sideband is created and registered.
> 
> Reported-by: Uttkarsh Aggarwal <uttkarsh.aggarwal@....qualcomm.com>
> Closes: https://lore.kernel.org/linux-usb/20251028080043.27760-1-uttkarsh.aggarwal@oss.qualcomm.com
> Fixes: de66754e9f80 ("xhci: sideband: add initial api to register a secondary interrupter entity")
> Signed-off-by: Mathias Nyman <mathias.nyman@...ux.intel.com>
> ---
> 
> v2:
>   use guard() and fix missing mutex_unlock as recommended by greg k-h 
> 
> ---
>  drivers/usb/host/xhci-sideband.c | 97 +++++++++++++++++---------------
>  1 file changed, 53 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-sideband.c b/drivers/usb/host/xhci-sideband.c
> index e771a476fef2..2daa0ba7ad9a 100644
> --- a/drivers/usb/host/xhci-sideband.c
> +++ b/drivers/usb/host/xhci-sideband.c
> @@ -86,6 +86,22 @@ __xhci_sideband_remove_endpoint(struct xhci_sideband *sb, struct xhci_virt_ep *e
>  	sb->eps[ep->ep_index] = NULL;
>  }
>  
> +static void
> +__xhci_sideband_remove_interrupter(struct xhci_sideband *sb)

This function must be called with the mutex locked, so shouldn't you
document that here so that the compiler can catch it if we mess up in
the future and forget to grab it?

> +{
> +	struct usb_device *udev;
> +
> +	if (!sb->ir)
> +		return;
> +
> +	xhci_remove_secondary_interrupter(xhci_to_hcd(sb->xhci), sb->ir);
> +	sb->ir = NULL;
> +	udev = sb->vdev->udev;
> +
> +	if (udev->state != USB_STATE_NOTATTACHED)
> +		usb_offload_put(udev);
> +}
> +
>  /* sideband api functions */
>  
>  /**
> @@ -131,14 +147,17 @@ xhci_sideband_add_endpoint(struct xhci_sideband *sb,
>  	struct xhci_virt_ep *ep;
>  	unsigned int ep_index;
>  
> -	mutex_lock(&sb->mutex);
> +	guard(mutex)(&sb->mutex);
> +
> +	if (!sb->vdev)
> +		return -ENODEV;
> +
>  	ep_index = xhci_get_endpoint_index(&host_ep->desc);
>  	ep = &sb->vdev->eps[ep_index];
>  
> -	if (ep->ep_state & EP_HAS_STREAMS) {
> -		mutex_unlock(&sb->mutex);
> +	if (ep->ep_state & EP_HAS_STREAMS)
>  		return -EINVAL;
> -	}
> +

Very minor nit, just delete the extra line, like you did in the rest of
the diff below here, otherwise you have 2 blank lines.

thanks,

greg k-h