linux-kernel - Re: [PATCH V2] usbip: Fix locking bug in RT-enabled kernels

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] [day] [month] [year] [list]

Message-ID: <9cacdd47-54ed-42cf-849f-14ed23156d0e@linuxfoundation.org>
Date: Wed, 17 Sep 2025 10:35:02 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: Lizhi Xu <lizhi.xu@...driver.com>, gregkh@...uxfoundation.org
Cc: linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
 shuah@...nel.org, stern@...land.harvard.edu,
 syzbot+205ef33a3b636b4181fb@...kaller.appspotmail.com,
 syzkaller-bugs@...glegroups.com, valentina.manea.m@...il.com,
 Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH V2] usbip: Fix locking bug in RT-enabled kernels

On 9/16/25 18:51, Lizhi Xu wrote:
> On Tue, 16 Sep 2025 17:33:44 -0600, Shuah Khan wrote:
>> On 9/16/25 09:47, Alan Stern wrote:
>>> On Tue, Sep 16, 2025 at 09:41:43AM +0800, Lizhi Xu wrote:
>>>> Interrupts are disabled before entering usb_hcd_giveback_urb().
>>>> A spinlock_t becomes a sleeping lock on PREEMPT_RT, so it cannot be
>>>> acquired with disabled interrupts.
>>>>
>>>> Save the interrupt status and restore it after usb_hcd_giveback_urb().
>>>>
>>>> syz reported:
>>>> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
>>>> Call Trace:
>>>>    dump_stack_lvl+0x189/0x250 lib/dump_stack.c:120
>>>>    rt_spin_lock+0xc7/0x2c0 kernel/locking/spinlock_rt.c:57
>>>>    spin_lock include/linux/spinlock_rt.h:44 [inline]
>>>>    mon_bus_complete drivers/usb/mon/mon_main.c:134 [inline]
>>>>    mon_complete+0x5c/0x200 drivers/usb/mon/mon_main.c:147
>>>>    usbmon_urb_complete include/linux/usb/hcd.h:738 [inline]
>>>>    __usb_hcd_giveback_urb+0x254/0x5e0 drivers/usb/core/hcd.c:1647
>>>>    vhci_urb_enqueue+0xb4f/0xe70 drivers/usb/usbip/vhci_hcd.c:818
>>>>
>>>> Reported-by: syzbot+205ef33a3b636b4181fb@...kaller.appspotmail.com
>>>> Closes: https://syzkaller.appspot.com/bug?extid=205ef33a3b636b4181fb
>>>> Signed-off-by: Lizhi Xu <lizhi.xu@...driver.com>
>>>> ---
>>>> V1 -> V2: fix it in usbip
>>>>
>>>>    drivers/usb/usbip/vhci_hcd.c | 6 +++---
>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
>>>> index e70fba9f55d6..eb6de7e8ea7b 100644
>>>> --- a/drivers/usb/usbip/vhci_hcd.c
>>>> +++ b/drivers/usb/usbip/vhci_hcd.c
>>>> @@ -809,15 +809,15 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>>>>    no_need_xmit:
>>>>    	usb_hcd_unlink_urb_from_ep(hcd, urb);
>>>>    no_need_unlink:
>>>> -	spin_unlock_irqrestore(&vhci->lock, flags);
>>>>    	if (!ret) {
>>>>    		/* usb_hcd_giveback_urb() should be called with
>>>>    		 * irqs disabled
>>>>    		 */
>>>> -		local_irq_disable();
>>>> +		spin_unlock(&vhci->lock);
>>>>    		usb_hcd_giveback_urb(hcd, urb, urb->status);
>>>> -		local_irq_enable();
>>>> +		spin_lock(&vhci->lock);
>>>>    	}
>>>> +	spin_unlock_irqrestore(&vhci->lock, flags);
>>>>    	return ret;
>>>>    }
>>>
>>> This looks right to me; it's the same pattern that the other host
>>> controller drivers use.  However, the final decision is up to the usbip
>>> maintainers.
>>>
>>> Also, there are several other places in the usbip drivers where
>>> usb_hcd_giveback_urb() gets called; shouldn't they all be changed to
>>> follow this pattern?
>>>
>>
>> Looks good to me.
>> +1 on changing all other instances - can we do that?
> I'm replying to both of you. Personally, I suggest this isn't necessary
> right now; it's safer to wait until a problem is reported before fixing it.
> 
> Also, the context of several other calls to usb_hcd_giveback_urb() in usbip
> differs from the current issue. Enabling RT_PREEMPT shouldn't cause similar
> issues.

Fair enough reasoning to wait. Here is my Acked by.

Acked-by: Shuah Khan <skhan@...uxfoundation.org>

Greg, please pick this up.

thanks,
-- Shuah