Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <CAAeHK+z2+_UHNp4_D2iL9FzPtDoU1YBohCaDJG8sAy12uc_-ew@mail.gmail.com>
 <Pine.LNX.4.44L0.2001131049090.1502-100000@iolanthe.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.2001131049090.1502-100000@iolanthe.rowland.org>
From:   Andrey Konovalov <andreyknvl@google.com>
Date:   Mon, 13 Jan 2020 18:34:27 +0100
Message-ID: <CAAeHK+x9Gk3cD77MA9jkhpwO8S62i6KT7PP3NZ6QTZ2qk2FB6w@mail.gmail.com>
Subject: Re: [PATCH v4 1/1] usb: gadget: add raw-gadget interface
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Felipe Balbi <balbi@kernel.org>,
        USB list <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Marco Elver <elver@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Jan 13, 2020 at 5:50 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, 13 Jan 2020, Andrey Konovalov wrote:
>
> > I've also found an issue, but I'm not sure if that is the bug in Raw
> > Gadget, or in the gadget layer (in the former case I'll add this fix
> > to v5 as well). What I believe I'm seeing is
> > __fput()->usb_gadget_unregister_driver()->usb_gadget_remove_driver()->gadget_unbind()
> > racing with dummy_timer()->gadget_setup(). In my case it results in
> > gadget_unbind() doing set_gadget_data(gadget, NULL), and then
> > gadget_setup() dereferencing get_gadget_data(gadget).
> >
> > Alan, does it look possible for those two functions to race? Should
> > this be prevented by the gadget layer, or should I use some kind of
> > locking in my gadget driver to prevent this?
>
> In your situation this race shouldn't happen, because before
> udc->driver->unbind() is invoked we call usb_gadget_disconnect().  If
> that routine succeeds -- which it always does under dummy-hcd -- then
> there can't be any more setup callbacks, because find_endpoint() will
> always return NULL (the is_active() test fails; see the various
> set_link_state* routines).  So I don't see how you could have ended up
> with the race you describe.

I've managed to reproduce the race by adding an mdelay() into the
beginning of the setup() callback. AFAIU what happens is setup() gets
called (and waits on the mdelay()), then unbind() comes in and does
set_gadget_data(NULL), and then setup() proceeds, gets NULL through
get_gadget_data() and crashes on null-ptr-deref. I've got the same
crash a few times after many days of fuzzing, so I assume it can
happen without the mdelay() as well.

> However, a real UDC might not be able to perform a disconnect under
> software control.  In that case usb_gadget_disconnect() would not
> change the pullup state, and there would be a real possibility of a
> setup callback racing with an unbind callback.  This seems like a
> genuine problem and I can't think of a solution offhand.
>
> What we would need is a way to tell the UDC driver to stop invoking
> gadget callbacks, _before_ the UDC driver's stop callback gets called.
> Maybe this should be merged into the pullup callback somehow.
>
> Alan Stern
>