From: Anirudh Rayabharam <mail@anirudhrb.com> To: Felipe Balbi <balbi@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Alan Stern <stern@rowland.harvard.edu>, Colin Ian King <colin.king@canonical.com>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, Jules Irenge <jbi.octave@gmail.com>, Bui Quang Minh <minhquangbui99@gmail.com>, Anirudh Rayabharam <mail@anirudhrb.com>, Andrey Konovalov <andreyknvl@gmail.com> Cc: linux-kernel-mentees@lists.linuxfoundation.org, syzbot+eb4674092e6cc8d9e0bd@syzkaller.appspotmail.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: [PATCH v2] usb: gadget: dummy_hcd: fix gpf in gadget_setup Date: Mon, 19 Apr 2021 09:07:08 +0530 [thread overview] Message-ID: <20210419033713.3021-1-mail@anirudhrb.com> (raw) Fix a general protection fault reported by syzbot due to a race between gadget_setup() and gadget_unbind() in raw_gadget. The gadget core is supposed to guarantee that there won't be any more callbacks to the gadget driver once the driver's unbind routine is called. That guarantee is enforced in usb_gadget_remove_driver as follows: usb_gadget_disconnect(udc->gadget); if (udc->gadget->irq) synchronize_irq(udc->gadget->irq); udc->driver->unbind(udc->gadget); usb_gadget_udc_stop(udc); usb_gadget_disconnect turns off the pullup resistor, telling the host that the gadget is no longer connected and preventing the transmission of any more USB packets. Any packets that have already been received are sure to processed by the UDC driver's interrupt handler by the time synchronize_irq returns. But this doesn't work with dummy_hcd, because dummy_hcd doesn't use interrupts; it uses a timer instead. It does have code to emulate the effect of synchronize_irq, but that code doesn't get invoked at the right time -- it currently runs in usb_gadget_udc_stop, after the unbind callback instead of before. Indeed, there's no way for usb_gadget_remove_driver to invoke this code before the unbind callback. To fix this, move the synchronize_irq() emulation code to dummy_pullup so that it runs before unbind. Also, add a comment explaining why it is necessary to have it there. Suggested-by: Alan Stern <stern@rowland.harvard.edu> Reported-by: syzbot+eb4674092e6cc8d9e0bd@syzkaller.appspotmail.com Acked-by: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> --- Changes in v2: Improvements in the comment as suggested by Alan Stern. v1: https://lore.kernel.org/lkml/20210417125212.6274-1-mail@anirudhrb.com/ --- drivers/usb/gadget/udc/dummy_hcd.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index ce24d4f28f2a..7db773c87379 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -903,6 +903,21 @@ static int dummy_pullup(struct usb_gadget *_gadget, int value) spin_lock_irqsave(&dum->lock, flags); dum->pullup = (value != 0); set_link_state(dum_hcd); + if (value == 0) { + /* + * Emulate synchronize_irq(): wait for callbacks to finish. + * This seems to be the best place to emulate the call to + * synchronize_irq() that's in usb_gadget_remove_driver(). + * Doing it in dummy_udc_stop() would be too late since it + * is called after the unbind callback and unbind shouldn't + * be invoked until all the other callbacks are finished. + */ + while (dum->callback_usage > 0) { + spin_unlock_irqrestore(&dum->lock, flags); + usleep_range(1000, 2000); + spin_lock_irqsave(&dum->lock, flags); + } + } spin_unlock_irqrestore(&dum->lock, flags); usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd)); @@ -1004,14 +1019,6 @@ static int dummy_udc_stop(struct usb_gadget *g) spin_lock_irq(&dum->lock); dum->ints_enabled = 0; stop_activity(dum); - - /* emulate synchronize_irq(): wait for callbacks to finish */ - while (dum->callback_usage > 0) { - spin_unlock_irq(&dum->lock); - usleep_range(1000, 2000); - spin_lock_irq(&dum->lock); - } - dum->driver = NULL; spin_unlock_irq(&dum->lock); -- 2.26.2
WARNING: multiple messages have this Message-ID (diff)
From: Anirudh Rayabharam <mail@anirudhrb.com> To: Felipe Balbi <balbi@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Alan Stern <stern@rowland.harvard.edu>, Colin Ian King <colin.king@canonical.com>, "Gustavo A. R. Silva" <gustavoars@kernel.org>, Jules Irenge <jbi.octave@gmail.com>, Bui Quang Minh <minhquangbui99@gmail.com>, Anirudh Rayabharam <mail@anirudhrb.com>, Andrey Konovalov <andreyknvl@gmail.com> Cc: syzbot+eb4674092e6cc8d9e0bd@syzkaller.appspotmail.com, linux-usb@vger.kernel.org, linux-kernel-mentees@lists.linuxfoundation.org, linux-kernel@vger.kernel.org Subject: [PATCH v2] usb: gadget: dummy_hcd: fix gpf in gadget_setup Date: Mon, 19 Apr 2021 09:07:08 +0530 [thread overview] Message-ID: <20210419033713.3021-1-mail@anirudhrb.com> (raw) Fix a general protection fault reported by syzbot due to a race between gadget_setup() and gadget_unbind() in raw_gadget. The gadget core is supposed to guarantee that there won't be any more callbacks to the gadget driver once the driver's unbind routine is called. That guarantee is enforced in usb_gadget_remove_driver as follows: usb_gadget_disconnect(udc->gadget); if (udc->gadget->irq) synchronize_irq(udc->gadget->irq); udc->driver->unbind(udc->gadget); usb_gadget_udc_stop(udc); usb_gadget_disconnect turns off the pullup resistor, telling the host that the gadget is no longer connected and preventing the transmission of any more USB packets. Any packets that have already been received are sure to processed by the UDC driver's interrupt handler by the time synchronize_irq returns. But this doesn't work with dummy_hcd, because dummy_hcd doesn't use interrupts; it uses a timer instead. It does have code to emulate the effect of synchronize_irq, but that code doesn't get invoked at the right time -- it currently runs in usb_gadget_udc_stop, after the unbind callback instead of before. Indeed, there's no way for usb_gadget_remove_driver to invoke this code before the unbind callback. To fix this, move the synchronize_irq() emulation code to dummy_pullup so that it runs before unbind. Also, add a comment explaining why it is necessary to have it there. Suggested-by: Alan Stern <stern@rowland.harvard.edu> Reported-by: syzbot+eb4674092e6cc8d9e0bd@syzkaller.appspotmail.com Acked-by: Alan Stern <stern@rowland.harvard.edu> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> --- Changes in v2: Improvements in the comment as suggested by Alan Stern. v1: https://lore.kernel.org/lkml/20210417125212.6274-1-mail@anirudhrb.com/ --- drivers/usb/gadget/udc/dummy_hcd.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/drivers/usb/gadget/udc/dummy_hcd.c b/drivers/usb/gadget/udc/dummy_hcd.c index ce24d4f28f2a..7db773c87379 100644 --- a/drivers/usb/gadget/udc/dummy_hcd.c +++ b/drivers/usb/gadget/udc/dummy_hcd.c @@ -903,6 +903,21 @@ static int dummy_pullup(struct usb_gadget *_gadget, int value) spin_lock_irqsave(&dum->lock, flags); dum->pullup = (value != 0); set_link_state(dum_hcd); + if (value == 0) { + /* + * Emulate synchronize_irq(): wait for callbacks to finish. + * This seems to be the best place to emulate the call to + * synchronize_irq() that's in usb_gadget_remove_driver(). + * Doing it in dummy_udc_stop() would be too late since it + * is called after the unbind callback and unbind shouldn't + * be invoked until all the other callbacks are finished. + */ + while (dum->callback_usage > 0) { + spin_unlock_irqrestore(&dum->lock, flags); + usleep_range(1000, 2000); + spin_lock_irqsave(&dum->lock, flags); + } + } spin_unlock_irqrestore(&dum->lock, flags); usb_hcd_poll_rh_status(dummy_hcd_to_hcd(dum_hcd)); @@ -1004,14 +1019,6 @@ static int dummy_udc_stop(struct usb_gadget *g) spin_lock_irq(&dum->lock); dum->ints_enabled = 0; stop_activity(dum); - - /* emulate synchronize_irq(): wait for callbacks to finish */ - while (dum->callback_usage > 0) { - spin_unlock_irq(&dum->lock); - usleep_range(1000, 2000); - spin_lock_irq(&dum->lock); - } - dum->driver = NULL; spin_unlock_irq(&dum->lock); -- 2.26.2 _______________________________________________ Linux-kernel-mentees mailing list Linux-kernel-mentees@lists.linuxfoundation.org https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees
next reply other threads:[~2021-04-19 3:38 UTC|newest] Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-19 3:37 Anirudh Rayabharam [this message] 2021-04-19 3:37 ` [PATCH v2] usb: gadget: dummy_hcd: fix gpf in gadget_setup Anirudh Rayabharam
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210419033713.3021-1-mail@anirudhrb.com \ --to=mail@anirudhrb.com \ --cc=andreyknvl@gmail.com \ --cc=balbi@kernel.org \ --cc=colin.king@canonical.com \ --cc=gregkh@linuxfoundation.org \ --cc=gustavoars@kernel.org \ --cc=jbi.octave@gmail.com \ --cc=linux-kernel-mentees@lists.linuxfoundation.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-usb@vger.kernel.org \ --cc=minhquangbui99@gmail.com \ --cc=stern@rowland.harvard.edu \ --cc=syzbot+eb4674092e6cc8d9e0bd@syzkaller.appspotmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.