Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751843AbdITSa1 (ORCPT ); Wed, 20 Sep 2017 14:30:27 -0400 Received: from mail-io0-f181.google.com ([209.85.223.181]:50425 "EHLO mail-io0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751588AbdITSaZ (ORCPT ); Wed, 20 Sep 2017 14:30:25 -0400 X-Google-Smtp-Source: AOwi7QDFnqZMjtgI0fCcObidimyiL2MEI3pnClAFw7IMtAzjdVmsK7YynEGra5Gz6ez9/BOaAwyekIO7yoSymrEW7I4= MIME-Version: 1.0 In-Reply-To: References: From: Andrey Konovalov Date: Wed, 20 Sep 2017 20:30:23 +0200 Message-ID: Subject: Re: usb/gadget: null-ptr-deref in dev_ioctl To: Alan Stern Cc: Felipe Balbi , Greg Kroah-Hartman , David Windsor , Andrew Morton , Elena Reshetova , Al Viro , Christophe JAILLET , Masahiro Yamada , USB list , LKML , Dmitry Vyukov , Kostya Serebryany , syzkaller Content-Type: multipart/mixed; boundary="001a11c1837c4503250559a32bcf" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8048 Lines: 205 --001a11c1837c4503250559a32bcf Content-Type: text/plain; charset="UTF-8" On Wed, Sep 20, 2017 at 7:59 PM, Alan Stern wrote: > On Mon, 11 Sep 2017, Andrey Konovalov wrote: > >> Hi! >> >> It seems that gadget->ops can be NULL so it probably needs to be >> checked as well as gadget->ops->ioctl in dev_ioctl() in >> drivers/usb/gadget/legacy/inode.c. > > Actually, I suspect the problem is that gadget is NULL, not > gadget->ops. Yes, you are correct, checked it by adding printk. Attaching a simple repro of the issue just for reference. > >> kasan: CONFIG_KASAN_INLINE enabled >> kasan: GPF could be caused by NULL-ptr deref or user memory access >> general protection fault: 0000 [#1] SMP KASAN >> Modules linked in: >> CPU: 1 PID: 5214 Comm: syz-executor Not tainted 4.13.0+ #94 >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 >> task: ffff88006ac0e800 task.stack: ffff88006af60000 >> ! handling hub events now: start >> RIP: 0010:dev_ioctl+0x117/0x280 drivers/usb/gadget/legacy/inode.c:1323 > > Can you test the patch below? I haven't tried it myself yet, but I > think it will fix the problem you found. This fixes the crash for me. Tested-by: Andrey Konovalov 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 > @@ -28,7 +28,7 @@ > #include > #include > #include > - > +#include > #include > #include > > @@ -116,6 +116,7 @@ enum ep0_state { > struct dev_data { > spinlock_t lock; > refcount_t count; > + int udc_usage; > enum ep0_state state; /* P: lock */ > struct usb_gadgetfs_event event [N_EVENT]; > unsigned ev_next; > @@ -513,9 +514,9 @@ static void ep_aio_complete(struct usb_e > INIT_WORK(&priv->work, ep_user_copy_worker); > schedule_work(&priv->work); > } > - spin_unlock(&epdata->dev->lock); > > usb_ep_free_request(ep, req); > + spin_unlock(&epdata->dev->lock); > put_ep(epdata); > } > > @@ -939,9 +940,11 @@ ep0_read (struct file *fd, char __user * > struct usb_request *req = dev->req; > > if ((retval = setup_req (ep, req, 0)) == 0) { > + ++dev->udc_usage; > spin_unlock_irq (&dev->lock); > retval = usb_ep_queue (ep, req, GFP_KERNEL); > spin_lock_irq (&dev->lock); > + --dev->udc_usage; > } > dev->state = STATE_DEV_CONNECTED; > > @@ -1131,6 +1134,7 @@ ep0_write (struct file *fd, const char _ > retval = setup_req (dev->gadget->ep0, dev->req, len); > if (retval == 0) { > dev->state = STATE_DEV_CONNECTED; > + ++dev->udc_usage; > spin_unlock_irq (&dev->lock); > if (copy_from_user (dev->req->buf, buf, len)) > retval = -EFAULT; > @@ -1142,6 +1146,7 @@ ep0_write (struct file *fd, const char _ > GFP_KERNEL); > } > spin_lock_irq(&dev->lock); > + --dev->udc_usage; > if (retval < 0) { > clean_req (dev->gadget->ep0, dev->req); > } else > @@ -1243,9 +1248,21 @@ static long dev_ioctl (struct file *fd, > struct usb_gadget *gadget = dev->gadget; > long ret = -ENOTTY; > > - if (gadget->ops->ioctl) > + spin_lock_irq(&dev->lock); > + if (dev->state == STATE_DEV_OPENED || > + dev->state == STATE_DEV_UNBOUND) { > + /* Not bound to a UDC */ > + } else if (gadget->ops->ioctl) { > + ++dev->udc_usage; > + spin_unlock_irq(&dev->lock); > + > ret = gadget->ops->ioctl (gadget, code, value); > > + spin_lock_irq(&dev->lock); > + --dev->udc_usage; > + } > + spin_unlock_irq(&dev->lock); > + > return ret; > } > > @@ -1463,10 +1480,12 @@ delegate: > if (value < 0) > break; > > + ++dev->udc_usage; > spin_unlock (&dev->lock); > value = usb_ep_queue (gadget->ep0, dev->req, > GFP_KERNEL); > spin_lock (&dev->lock); > + --dev->udc_usage; > if (value < 0) { > clean_req (gadget->ep0, dev->req); > break; > @@ -1490,8 +1509,12 @@ delegate: > req->length = value; > req->zero = value < w_length; > > + ++dev->udc_usage; > spin_unlock (&dev->lock); > value = usb_ep_queue (gadget->ep0, req, GFP_KERNEL); > + spin_lock(&dev->lock); > + --dev->udc_usage; > + spin_unlock(&dev->lock); > if (value < 0) { > DBG (dev, "ep_queue --> %d\n", value); > req->status = 0; > @@ -1518,21 +1541,24 @@ static void destroy_ep_files (struct dev > /* break link to FS */ > ep = list_first_entry (&dev->epfiles, struct ep_data, epfiles); > list_del_init (&ep->epfiles); > + spin_unlock_irq (&dev->lock); > + > dentry = ep->dentry; > ep->dentry = NULL; > parent = d_inode(dentry->d_parent); > > /* break link to controller */ > + mutex_lock(&ep->lock); > if (ep->state == STATE_EP_ENABLED) > (void) usb_ep_disable (ep->ep); > ep->state = STATE_EP_UNBOUND; > usb_ep_free_request (ep->ep, ep->req); > ep->ep = NULL; > + mutex_unlock(&ep->lock); > + > wake_up (&ep->wait); > put_ep (ep); > > - spin_unlock_irq (&dev->lock); > - > /* break link to dcache */ > inode_lock(parent); > d_delete (dentry); > @@ -1603,6 +1629,11 @@ gadgetfs_unbind (struct usb_gadget *gadg > > spin_lock_irq (&dev->lock); > dev->state = STATE_DEV_UNBOUND; > + while (dev->udc_usage > 0) { > + spin_unlock_irq(&dev->lock); > + usleep_range(1000, 2000); > + spin_lock_irq(&dev->lock); > + } > spin_unlock_irq (&dev->lock); > > destroy_ep_files (dev); > --001a11c1837c4503250559a32bcf Content-Type: text/x-csrc; charset="US-ASCII"; name="gfs-ioctl-null-poc.c" Content-Disposition: attachment; filename="gfs-ioctl-null-poc.c" Content-Transfer-Encoding: base64 X-Attachment-Id: f_j7td5cgj0 I2luY2x1ZGUgPGZjbnRsLmg+CiNpbmNsdWRlIDxzeXMvaW9jdGwuaD4KI2luY2x1ZGUgPHN5cy9z dGF0Lmg+CiNpbmNsdWRlIDxzeXMvdHlwZXMuaD4KCmludCBtYWluKCkgewoJc3lzdGVtKCJta2Rp ciAvZGV2L2dhZGdldCAmJiBtb3VudCAtdCBnYWRnZXRmcyBub25lIC9kZXYvZ2FkZ2V0Iik7Cglp bnQgZmQgPSBvcGVuKCIvZGV2L2dhZGdldC9kdW1teV91ZGMiLCBPX1JEV1IpOwoJaW9jdGwoZmQs IDAsIDApOwoJcmV0dXJuIDA7Cn0K --001a11c1837c4503250559a32bcf--