[PATCH] usb: usbip: add missing device lock on tweak configuration cmd

[PATCH] usb: usbip: add missing device lock on tweak configuration cmd

[Niels Dossche]

The function documentation of usb_set_configuration says that its
callers should hold the device lock. This lock is held for all
callsites except tweak_set_configuration_cmd. The code path can be
executed for example when attaching a remote USB device.
The solution is to surround the call by the device lock.

This bug was found using my experimental own-developed static analysis
tool, which reported the missing lock on v5.17.2. I manually verified
this bug report by doing code review as well. I runtime checked that
the required lock is not held. I compiled and runtime tested this on
x86_64 with a USB mouse. After applying this patch, my analyser no
longer reports this potential bug.

Fixes: 2c8c98158946 ("staging: usbip: let client choose device configuration")
Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
---

I'm developing this tool as part of my master's dissertation in order to
obtain my master's degree. If you'd like more information about the
details of the tool, please let me know.

 drivers/usb/usbip/stub_rx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
index 325c22008e53..5dd41e8215e0 100644
--- a/drivers/usb/usbip/stub_rx.c
+++ b/drivers/usb/usbip/stub_rx.c
@@ -138,7 +138,9 @@ static int tweak_set_configuration_cmd(struct urb *urb)
 	req = (struct usb_ctrlrequest *) urb->setup_packet;
 	config = le16_to_cpu(req->wValue);
 
+	usb_lock_device(sdev->udev);
 	err = usb_set_configuration(sdev->udev, config);
+	usb_unlock_device(sdev->udev);
 	if (err && err != -ENODEV)
 		dev_err(&sdev->udev->dev, "can't set config #%d, error %d\n",
 			config, err);
-- 
2.35.1

Re: [PATCH] usb: usbip: add missing device lock on tweak configuration cmd

[Shuah Khan]

On 4/12/22 10:50 AM, Niels Dossche wrote:
> The function documentation of usb_set_configuration says that its
> callers should hold the device lock. This lock is held for all
> callsites except tweak_set_configuration_cmd. The code path can be
> executed for example when attaching a remote USB device.
> The solution is to surround the call by the device lock.
> 
> This bug was found using my experimental own-developed static analysis
> tool, which reported the missing lock on v5.17.2. I manually verified
> this bug report by doing code review as well. I runtime checked that
> the required lock is not held. I compiled and runtime tested this on
> x86_64 with a USB mouse. After applying this patch, my analyser no
> longer reports this potential bug.
> 

How did you run-time check that the lock isn't held? Also x86_64 with a
USB mouse - did you test it over loopback?

> Fixes: 2c8c98158946 ("staging: usbip: let client choose device configuration")
> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
> ---
> 
> I'm developing this tool as part of my master's dissertation in order to
> obtain my master's degree. If you'd like more information about the
> details of the tool, please let me know.
> 
>   drivers/usb/usbip/stub_rx.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
> index 325c22008e53..5dd41e8215e0 100644
> --- a/drivers/usb/usbip/stub_rx.c
> +++ b/drivers/usb/usbip/stub_rx.c
> @@ -138,7 +138,9 @@ static int tweak_set_configuration_cmd(struct urb *urb)
>   	req = (struct usb_ctrlrequest *) urb->setup_packet;
>   	config = le16_to_cpu(req->wValue);
>   
> +	usb_lock_device(sdev->udev);
>   	err = usb_set_configuration(sdev->udev, config);
> +	usb_unlock_device(sdev->udev);
>   	if (err && err != -ENODEV)
>   		dev_err(&sdev->udev->dev, "can't set config #%d, error %d\n",
>   			config, err);
> 

Looks good to me with the above questions answered.

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

Greg, please pick this patch up.

thanks,
-- Shuah

Re: [PATCH] usb: usbip: add missing device lock on tweak configuration cmd

[Niels Dossche]

On 4/12/22 21:45, Shuah Khan wrote:
> On 4/12/22 10:50 AM, Niels Dossche wrote:
>> The function documentation of usb_set_configuration says that its
>> callers should hold the device lock. This lock is held for all
>> callsites except tweak_set_configuration_cmd. The code path can be
>> executed for example when attaching a remote USB device.
>> The solution is to surround the call by the device lock.
>>
>> This bug was found using my experimental own-developed static analysis
>> tool, which reported the missing lock on v5.17.2. I manually verified
>> this bug report by doing code review as well. I runtime checked that
>> the required lock is not held. I compiled and runtime tested this on
>> x86_64 with a USB mouse. After applying this patch, my analyser no
>> longer reports this potential bug.
>>
> 
> How did you run-time check that the lock isn't held? Also x86_64 with a
> USB mouse - did you test it over loopback?

I checked the lock isn't held with lockdep_assert_not_held and also by
checking the return value of usb_trylock_device to be equal to 1 in my testing.
I checked my USB mouse over loopback yes, binding the mouse for remote use and
attaching on loopback.

Thank you
Niels

> 
>> Fixes: 2c8c98158946 ("staging: usbip: let client choose device configuration")
>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>> ---
>>
>> I'm developing this tool as part of my master's dissertation in order to
>> obtain my master's degree. If you'd like more information about the
>> details of the tool, please let me know.
>>
>>   drivers/usb/usbip/stub_rx.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c
>> index 325c22008e53..5dd41e8215e0 100644
>> --- a/drivers/usb/usbip/stub_rx.c
>> +++ b/drivers/usb/usbip/stub_rx.c
>> @@ -138,7 +138,9 @@ static int tweak_set_configuration_cmd(struct urb *urb)
>>       req = (struct usb_ctrlrequest *) urb->setup_packet;
>>       config = le16_to_cpu(req->wValue);
>>   +    usb_lock_device(sdev->udev);
>>       err = usb_set_configuration(sdev->udev, config);
>> +    usb_unlock_device(sdev->udev);
>>       if (err && err != -ENODEV)
>>           dev_err(&sdev->udev->dev, "can't set config #%d, error %d\n",
>>               config, err);
>>
> 
> Looks good to me with the above questions answered.
> 
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> 
> Greg, please pick this patch up.
> 
> thanks,
> -- Shuah

Re: [PATCH] usb: usbip: add missing device lock on tweak configuration cmd

[Shuah Khan]

On 4/12/22 2:01 PM, Niels Dossche wrote:
> On 4/12/22 21:45, Shuah Khan wrote:
>> On 4/12/22 10:50 AM, Niels Dossche wrote:
>>> The function documentation of usb_set_configuration says that its
>>> callers should hold the device lock. This lock is held for all
>>> callsites except tweak_set_configuration_cmd. The code path can be
>>> executed for example when attaching a remote USB device.
>>> The solution is to surround the call by the device lock.
>>>
>>> This bug was found using my experimental own-developed static analysis
>>> tool, which reported the missing lock on v5.17.2. I manually verified
>>> this bug report by doing code review as well. I runtime checked that
>>> the required lock is not held. I compiled and runtime tested this on
>>> x86_64 with a USB mouse. After applying this patch, my analyser no
>>> longer reports this potential bug.
>>>
>>
>> How did you run-time check that the lock isn't held? Also x86_64 with a
>> USB mouse - did you test it over loopback?
> 
> I checked the lock isn't held with lockdep_assert_not_held and also by
> checking the return value of usb_trylock_device to be equal to 1 in my testing.
> I checked my USB mouse over loopback yes, binding the mouse for remote use and
> attaching on loopback.
> 

Great. Thank you for a detailed answer.

> 
>>
>>> Fixes: 2c8c98158946 ("staging: usbip: let client choose device configuration")
>>> Signed-off-by: Niels Dossche <dossche.niels@gmail.com>
>>> ---
>>>

>>
>> Looks good to me with the above questions answered.
>>
>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>>
>> Greg, please pick this patch up.
>>

thanks,
-- Shuah