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

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Ynay6XK+ZUWtvfbH@rowland.harvard.edu>
Date:   Sat, 7 May 2022 13:56:57 -0400
From:   Alan Stern <stern@...land.harvard.edu>
To:     Schspa Shi <schspa@...il.com>
Cc:     Julia.Lawall@...ia.fr, andreyknvl@...il.com, balbi@...nel.org,
        gregkh@...uxfoundation.org, jannh@...gle.com,
        jj251510319013@...il.com, linux-kernel@...r.kernel.org,
        linux-usb@...r.kernel.org,
        syzbot+dc7c3ca638e773db07f6@...kaller.appspotmail.com
Subject: Re: [PATCH v2] usb: gadget: fix race when gadget driver register via
 ioctl

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@...kaller.appspotmail.com
> Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/
> 
> Signed-off-by: Schspa Shi <schspa@...il.com>
> ---
>  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)
>