USB: core: Fix unlocked hcd->status_urb access in usb_hcd_poll_rh_status()

[PATCH] USB: core: Fix unlocked hcd->status_urb access in usb_hcd_poll_rh_status()

Posted by Ginger Li 1 day, 4 hours ago

usb_hcd_poll_rh_status() tests hcd->status_urb without holding
hcd_root_hub_lock, while rh_queue_status() and the completion path inside
usb_hcd_poll_rh_status() itself update that field under the lock.

Take hcd_root_hub_lock around the early bail-out check so that the read is
serialized with the writers.

Fixes: d5926ae7a827 ("[PATCH] usbcore support for root-hub IRQ instead of polling")
Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
---
 drivers/usb/core/hcd.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -727,8 +727,13 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
 
 	if (unlikely(!hcd->rh_pollable))
 		return;
-	if (!hcd->uses_new_polling && !hcd->status_urb)
+
+	spin_lock_irqsave(&hcd_root_hub_lock, flags);
+	if (!hcd->uses_new_polling && !hcd->status_urb) {
+		spin_unlock_irqrestore(&hcd_root_hub_lock, flags);
 		return;
+	}
+	spin_unlock_irqrestore(&hcd_root_hub_lock, flags);
 
 	length = hcd->driver->hub_status_data(hcd, buffer);
 	if (length > 0) {
-- 
2.43.0

Re: [PATCH] USB: core: Fix unlocked hcd->status_urb access in usb_hcd_poll_rh_status()

Posted by Alan Stern 22 hours ago

On Wed, Sep 23, 2026 at 04:38:28PM +0800, Ginger Li wrote:
> usb_hcd_poll_rh_status() tests hcd->status_urb without holding
> hcd_root_hub_lock, while rh_queue_status() and the completion path inside
> usb_hcd_poll_rh_status() itself update that field under the lock.
> 
> Take hcd_root_hub_lock around the early bail-out check so that the read is
> serialized with the writers.

Why does the read need to be serialized with the writers?  What problems 
does lack of serialization cause?

Alan Stern

> Fixes: d5926ae7a827 ("[PATCH] usbcore support for root-hub IRQ instead of polling")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
>  drivers/usb/core/hcd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -727,8 +727,13 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>  
>  	if (unlikely(!hcd->rh_pollable))
>  		return;
> -	if (!hcd->uses_new_polling && !hcd->status_urb)
> +
> +	spin_lock_irqsave(&hcd_root_hub_lock, flags);
> +	if (!hcd->uses_new_polling && !hcd->status_urb) {
> +		spin_unlock_irqrestore(&hcd_root_hub_lock, flags);
>  		return;
> +	}
> +	spin_unlock_irqrestore(&hcd_root_hub_lock, flags);
>  
>  	length = hcd->driver->hub_status_data(hcd, buffer);
>  	if (length > 0) {
> -- 
> 2.43.0
> 
>

Re: [PATCH] USB: core: Fix unlocked hcd->status_urb access in usb_hcd_poll_rh_status()

Posted by Greg KH 1 day, 3 hours ago

On Wed, Sep 23, 2026 at 04:38:28PM +0800, Ginger Li wrote:
> usb_hcd_poll_rh_status() tests hcd->status_urb without holding
> hcd_root_hub_lock, while rh_queue_status() and the completion path inside
> usb_hcd_poll_rh_status() itself update that field under the lock.
> 
> Take hcd_root_hub_lock around the early bail-out check so that the read is
> serialized with the writers.

Is that a real issue?  How can this be hit?

How was this found?  How was it tested?
Did you forget an Assisted-by tag?



> 
> Fixes: d5926ae7a827 ("[PATCH] usbcore support for root-hub IRQ instead of polling")
> Signed-off-by: Ginger Li <ginger.jzllee@gmail.com>
> ---
>  drivers/usb/core/hcd.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -727,8 +727,13 @@ void usb_hcd_poll_rh_status(struct usb_hcd *hcd)
>  
>  	if (unlikely(!hcd->rh_pollable))
>  		return;
> -	if (!hcd->uses_new_polling && !hcd->status_urb)
> +
> +	spin_lock_irqsave(&hcd_root_hub_lock, flags);

guard()?

thanks,

greg k-h