Re: KASAN: use-after-free Write in hiddev_disconnect

From: Dan Carpenter <dan.carpenter@oracle.com>
To: syzbot <syzbot+784ccb935f9900cc7c9e@syzkaller.appspotmail.com>
Cc: andreyknvl@google.com, benjamin.tissoires@redhat.com,
	jikos@kernel.org, linux-input@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	syzkaller-bugs@googlegroups.com
Subject: Re: KASAN: use-after-free Write in hiddev_disconnect
Date: Tue, 14 Jan 2020 18:24:14 +0300	[thread overview]
Message-ID: <20200114152414.GC3719@kadam> (raw)
In-Reply-To: <0000000000004a11c1059c193a54@google.com>

I'm trying to take a stab at diagnosing these bugs.  (But I'm seldom
smart enough to actually fix anything).  These hiddev_disconnect() bugs
are a race condition:

devel/drivers/hid/usbhid/hiddev.c
   924  void hiddev_disconnect(struct hid_device *hid)
   925  {
   926          struct hiddev *hiddev = hid->hiddev;
   927          struct usbhid_device *usbhid = hid->driver_data;
   928  
   929          usb_deregister_dev(usbhid->intf, &hiddev_class);
   930  
   931          mutex_lock(&hiddev->existancelock);
   932          hiddev->exist = 0;
                ^^^^^^^^^^^^^^^^^^
We set "exist = 0;"

   933  
   934          if (hiddev->open) {
   935                  mutex_unlock(&hiddev->existancelock);
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Then we drop the lock.

   936                  hid_hw_close(hiddev->hid);
   937                  wake_up_interruptible(&hiddev->wait);
                                               ^^^^^^
The other thread frees hiddev and it crashes here.

   938          } else {
   939                  mutex_unlock(&hiddev->existancelock);
   940                  kfree(hiddev);
   941          }
   942  }

The other thread is doing:

drivers/hid/usbhid/hiddev.c
   216  static int hiddev_release(struct inode * inode, struct file * file)
   217  {
   218          struct hiddev_list *list = file->private_data;
   219          unsigned long flags;
   220  
   221          spin_lock_irqsave(&list->hiddev->list_lock, flags);
   222          list_del(&list->node);
   223          spin_unlock_irqrestore(&list->hiddev->list_lock, flags);
   224  
   225          mutex_lock(&list->hiddev->existancelock);
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^
Takes the lock.

   226          if (!--list->hiddev->open) {
                     ^^^^^^^^^^^^^^^^^^^^
Decrements open.

   227                  if (list->hiddev->exist) {
                            ^^^^^^^^^^^^^^^^^^^
This is false.

   228                          hid_hw_close(list->hiddev->hid);
   229                          hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL);
   230                  } else {
   231                          mutex_unlock(&list->hiddev->existancelock);
   232                          kfree(list->hiddev);
                                      ^^^^^^^^^^^^
Freed here.

   233                          vfree(list);
   234                          return 0;
   235                  }
   236          }
   237  
   238          mutex_unlock(&list->hiddev->existancelock);
   239          vfree(list);
   240  
   241          return 0;
   242  }

I'm not sure what the fix should be though.

regards,
dan carpenter