Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <YnfVEcOWO63uIGs5@rowland.harvard.edu> <20220508150247.38204-1-schspa@gmail.com>
In-Reply-To: <20220508150247.38204-1-schspa@gmail.com>
From:   Andrey Konovalov <andreyknvl@gmail.com>
Date:   Mon, 9 May 2022 01:51:09 +0200
Message-ID: <CA+fCnZexPtGYX0kiwpA3-vgRbqbw0hk-fRpz9jUZrykJj-HN=w@mail.gmail.com>
Subject: Re: [PATCH v3] usb: gadget: fix race when gadget driver register via ioctl
To:     Schspa Shi <schspa@gmail.com>
Cc:     stern@rowland.harvard.edu, Julia Lawall <Julia.Lawall@inria.fr>,
        Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Jann Horn <jannh@google.com>,
        Wei Ming Chen <jj251510319013@gmail.com>,
        LKML <linux-kernel@vger.kernel.org>,
        USB list <linux-usb@vger.kernel.org>,
        syzbot+dc7c3ca638e773db07f6@syzkaller.appspotmail.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, May 8, 2022 at 5:03 PM Schspa Shi <schspa@gmail.com> 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.
>
> Please refer to the following scenarios.
>
>            T1                                  T2
> ------------------------------------------------------------------
> usb_gadget_register_driver_owner
>   driver_register                    driver_register
>     driver_find                       driver_find
>     bus_add_driver                    bus_add_driver
>       priv alloced                     <context switch>
>       drv->p = priv;
>       <schedule out>
>       kobject_init_and_add // refcount = 1;
>    //couldn't find an available UDC or it's busy
>    <context switch>
>                                        priv alloced
>                                        drv->priv = priv;
>                                        kobject_init_and_add
>                                          ---> refcount = 1 <------
>                                        // register success
>                                        <context switch>
> ===================== another ioctl/process ======================
>                                       driver_register
>                                        driver_find
>                                         k = kset_find_obj()
>                                          ---> refcount = 2 <------
>                                         <context out>
>    driver_unregister
>    // drv->p become T2's priv
>    ---> refcount = 1 <------
>    <context switch>
>                                         kobject_put(k)
>                                          ---> refcount = 0 <------
>                                         return priv->driver;
>                                         --------UAF here----------
>
> There will be UAF in this scenario.
>
> We can fix it by adding a new STATE_DEV_REGISTERING device state to
> avoid double register.
>
> Reported-by: syzbot+dc7c3ca638e773db07f6@syzkaller.appspotmail.com
> Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/
>
> Signed-off-by: Schspa Shi <schspa@gmail.com>
>
> ---
>
> Changelog:
> v1 -> v2:
>         - Add a STATE_DEV_REGISTERING as Alan Stern suggested.
> v2 -> v3:
>         - Adjust STATE_DEV_REGISTERING position to reflect the actual
>           order in which the states occur.
>         - Add the fault scenarios to comments.
> ---
>  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..241740024c50 100644
> --- a/drivers/usb/gadget/legacy/raw_gadget.c
> +++ b/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -145,6 +145,7 @@ enum dev_state {
>         STATE_DEV_INVALID = 0,
>         STATE_DEV_OPENED,
>         STATE_DEV_INITIALIZED,
> +       STATE_DEV_REGISTERING,
>         STATE_DEV_RUNNING,
>         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)
>

Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com>

Thanks, Schspa!

Thanks for the review, Alan!