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

[Niels Dossche <dossche.niels@gmail.com>]

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