Re: [PATCH v2] usb: gadget: fix race when gadget driver register via ioctl

[Alan Stern]

On Sun, May 08, 2022 at 12:02:43AM +0800, Schspa Shi wrote:
> The usb_gadget_register_driver can be called multi time by to
> threads via USB_RAW_IOCTL_RUN ioctl syscall, which will lead
> to multiple registrations.
> 
> Call trace:
>   driver_register+0x220/0x3a0 drivers/base/driver.c:171
>   usb_gadget_register_driver_owner+0xfb/0x1e0
>     drivers/usb/gadget/udc/core.c:1546
>   raw_ioctl_run drivers/usb/gadget/legacy/raw_gadget.c:513 [inline]
>   raw_ioctl+0x1883/0x2730 drivers/usb/gadget/legacy/raw_gadget.c:1220
>   ioctl USB_RAW_IOCTL_RUN
> 
> This routine allows two processes to register the same driver instance
> via ioctl syscall. which lead to a race condition.
> 
> We can fix it by adding a new STATE_DEV_REGISTERING device state to
> avoid double register.

Are you sure that this patch will fix the problem found by syzbot?  That 
is, are you sure that the problem really was caused by two threads 
registering the same driver concurrently?

The fact that the error was "use after free" suggests that the problem 
might be something else.  It looks like one of the threads was trying to 
access the driver structure after the other thread had done something 
that caused it to be deallocated, which suggests an error in reference 
counting.

Yes, this could be caused by two threads both registering the same 
driver.  But the evidence doesn't prove that this is what happened, as 
far as I can see.

Alan Stern

> Reported-by: syzbot+dc7c3ca638e773db07f6@xxxxxxxxxxxxxxxxxxxxxxxxx
> Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@xxxxxxxxxx/
> 
> Signed-off-by: Schspa Shi <schspa@xxxxxxxxx>
> ---
>  drivers/usb/gadget/legacy/raw_gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
> index b3be8db1ff63..b75f8f7b7b46 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -146,6 +146,7 @@ enum dev_state {
>  	STATE_DEV_OPENED,
>  	STATE_DEV_INITIALIZED,
>  	STATE_DEV_RUNNING,
> +	STATE_DEV_REGISTERING,
>  	STATE_DEV_CLOSED,
>  	STATE_DEV_FAILED
>  };
> @@ -508,6 +509,7 @@ static int raw_ioctl_run(struct raw_dev *dev, unsigned long value)
>  		ret = -EINVAL;
>  		goto out_unlock;
>  	}
> +	dev->state = STATE_DEV_REGISTERING;
>  	spin_unlock_irqrestore(&dev->lock, flags);
>  
>  	ret = usb_gadget_register_driver(&dev->driver);
> -- 
> 2.24.3 (Apple Git-128)
>