From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752099AbdFHQez (ORCPT ); Thu, 8 Jun 2017 12:34:55 -0400 Received: from mail-yw0-f178.google.com ([209.85.161.178]:34633 "EHLO mail-yw0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751876AbdFHQex (ORCPT ); Thu, 8 Jun 2017 12:34:53 -0400 MIME-Version: 1.0 In-Reply-To: References: From: Andrey Konovalov Date: Thu, 8 Jun 2017 18:34:51 +0200 Message-ID: Subject: Re: usb/gadget: another GPF in usb_gadget_unregister_driver To: Alan Stern Cc: Felipe Balbi , Greg Kroah-Hartman , Peter Chen , Krzysztof Opasiak , Colin Ian King , =?UTF-8?Q?Felix_H=C3=A4dicke?= , Roger Quadros , USB list , LKML , Dmitry Vyukov , Kostya Serebryany , syzkaller Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 8, 2017 at 5:55 PM, Alan Stern wrote: > On Thu, 8 Jun 2017, Andrey Konovalov wrote: > >> On Wed, Jun 7, 2017 at 11:20 PM, Alan Stern wrote: >> > On Wed, 7 Jun 2017, Andrey Konovalov wrote: >> > >> >> On Wed, Jun 7, 2017 at 4:43 PM, Alan Stern 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. >