linux-kernel - Re: usb/gadget: another GPF in usb_gadget_unregister

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] [day] [month] [year] [list]
Date:   Thu, 8 Jun 2017 18:34:51 +0200
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Peter Chen <peter.chen@....com>,
        Krzysztof Opasiak <k.opasiak@...sung.com>,
        Colin Ian King <colin.king@...onical.com>,
        Felix Hädicke <felixhaedicke@....de>,
        Roger Quadros <rogerq@...com>,
        USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: usb/gadget: another GPF in usb_gadget_unregister_driver

On Thu, Jun 8, 2017 at 5:55 PM, Alan Stern <stern@...land.harvard.edu> wrote:
> On Thu, 8 Jun 2017, Andrey Konovalov wrote:
>
>> On Wed, Jun 7, 2017 at 11:20 PM, Alan Stern <stern@...land.harvard.edu> wrote:
>> > On Wed, 7 Jun 2017, Andrey Konovalov wrote:
>> >
>> >> On Wed, Jun 7, 2017 at 4:43 PM, Alan Stern <stern@...land.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.
>
> Good, that's definitive.  And I feel stupid for missing this bug.
> The patch is below.

Perfect, this fixes the issue, thanks!

>
> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/legacy/inode.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c
> +++ usb-4.x/drivers/usb/gadget/legacy/inode.c
> @@ -1183,8 +1183,10 @@ dev_release (struct inode *inode, struct
>
>         /* closing ep0 === shutdown all */
>
> -       if (dev->gadget_registered)
> +       if (dev->gadget_registered) {
>                 usb_gadget_unregister_driver (&gadgetfs_driver);
> +               dev->gadget_registered = false;
> +       }
>
>         /* at this point "good" hardware has disconnected the
>          * device from USB; the host won't see it any more.
>