Re: usb/gadget: null-ptr-deref in dev_ioctl

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 20, 2017 at 7:59 PM, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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 <andreyknvl@xxxxxxxxxx>

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 <linux/aio.h>
>  #include <linux/uio.h>
>  #include <linux/refcount.h>
> -
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/moduleparam.h>
>
> @@ -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);
>
#include <fcntl.h>
#include <sys/ioctl.h>
#include <sys/stat.h>
#include <sys/types.h>

int main() {
	system("mkdir /dev/gadget && mount -t gadgetfs none /dev/gadget");
	int fd = open("/dev/gadget/dummy_udc", O_RDWR);
	ioctl(fd, 0, 0);
	return 0;
}

[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux