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: <20250917005136.1801295-1-lizhi.xu@windriver.com>
Date: Wed, 17 Sep 2025 08:51:36 +0800
From: Lizhi Xu <lizhi.xu@...driver.com>
To: <skhan@...uxfoundation.org>
CC: <gregkh@...uxfoundation.org>, <linux-kernel@...r.kernel.org>,
        <linux-usb@...r.kernel.org>, <lizhi.xu@...driver.com>,
        <shuah@...nel.org>, <stern@...land.harvard.edu>,
        <syzbot+205ef33a3b636b4181fb@...kaller.appspotmail.com>,
        <syzkaller-bugs@...glegroups.com>, <valentina.manea.m@...il.com>
Subject: Re: [PATCH V2] usbip: Fix locking bug in RT-enabled kernels

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.

BR,
Lizhi