Re: usb/gadget: another GPF in usb_gadget_unregister_driver

[Andrey Konovalov <andreyknvl@google.com>]

On Wed, Jun 7, 2017 at 11:20 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, 7 Jun 2017, Andrey Konovalov wrote:
>
>> On Wed, Jun 7, 2017 at 4:43 PM, Alan Stern <stern@rowland.harvard.edu> wrote:
>> > On Wed, 7 Jun 2017, Andrey Konovalov wrote:
>> >
>> >> Hi,
>> >>
>> >> I've got the following error report while fuzzing the kernel with syzkaller.
>> >>
>> >> On commit b29794ec95c6856b316c2295904208bf11ffddd9 (4.12-rc4+).
>> >>
>> >> This looks quite similar to
>> >> https://groups.google.com/forum/#!topic/syzkaller/HDawLBeeORI
>> >
>> > It does look very similar, but that problem was supposed to have been
>> > fixed by commit 7b0173811260 ("usb: gadget: udc: core: fix return code
>> > of usb_gadget_probe_driver()").
>> >
>> >> I'm able to reproduce this, so I can collect some debug traces if needed.
>> >
>> > Can you provide an strace or the equivalent?
>>
>> Here's the syzkaller program (which is actually two programs executed
>> consequently):
>> https://gist.github.com/xairy/fe0a7531e00df5e8bc23e2e56e413510
>>
>> Here's the strace log:
>> https://gist.github.com/xairy/5fadc3b5d8b2b80c97e566538de08bc4
>
> Do you know which of the two programs got the GPF?  I can't tell from
> the strace log.
>
>> Unfortunately there's a lot of unrelated garbage, but I can't extract
>> a simple C reproducer.
>
> That's okay, it's easy enough to see what's going on.  One program
> opens /dev/gadget/dummy_udc, writes an invalid setup string, then
> writes a valid setup string, and then exits.  The other program just
> opens the file and then exits.
>
>> I can also apply patches with debug printk's, run the reproducer and
>> send you the result if that will help.

I've extract another crash log, which is a little simpler:
https://gist.github.com/xairy/b8c814cbd731e4632e8e8fa0f51a29e8

>
> Maybe you can patch usb_gadget_probe_driver() in
> drivers/usb/gadget/udc/core.c.  Find out whether the "if
> (!driver->match_existing_only)" test is executed and whether it
> succeeds, and find out whether the code following "found:" is executed.
> I would expect that the test is not executed and the jump to "found:"
> is taken, so udc_bind_to_driver() is called and returns 0.  Thus,
> udc->driver should be set to driver.

Here's the funcgraph for usb_gadget_probe_driver:
https://gist.github.com/xairy/3221e2cb9c59514880d24c955de30b80

The (!driver->match_existing_only) test is not executed.
The code following "found:" is executed.

>
> Also, in usb_gadget_unregister_driver(), in the list_for_each_entry()
> loop, we should have udc->driver == driver and therefore ret should get
> set to 0.  Consequently, the list_del() near the end should not be
> executed and so the GPF should not occur.

Here's the funcgraph for usb_gadget_unregister_driver:
https://gist.github.com/xairy/887c52a12af8c9f9fe8ba3e4fa0ef1f0

What you described happens during the first call of
usb_gadget_unregister_driver(), however there's another one after
that, which is probably triggered by the second program.

>
> In particular, do these subroutines get called more than once?

usb_gadget_unregister_driver() is called twice, the GPF happens during
the second call.

>
> Alan Stern
>