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

[Shuah Khan <skhan@linuxfoundation.org>]

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