Skip to contentSkip to conversation

usb: raw_gadget: Fix a KASAN double-free bug in raw_release

PATCH In review Chang Yu 2 participants 2 messages Nov 2, 2024 – Nov 3, 2024

Subsystems (2)
USB USB raw gadget driver
lore
Share & tools
lore

Chang Yu

author
#0 lore raw

PATCH 1/1 usb: raw_gadget: Fix a KASAN double-free bug in raw_release

syzkaller reported a double free bug
(https://syzkaller.appspot.com/bug?extid=3e563d99e70973c0755c) in raw_release.

I suspect this is because a race between raw_release and raw_ioctl_run. While kref_get in raw_ioctl_run is protected by the spin lock, all kref_put in raw_release are not under the lock. This makes it possible that a kref_put might occur during kref_get, which is specifically prohibited by the kref documentation[1].

The fix is to ensure that all kref_put calls are made under lock and that we only call kfree(dev) after releasing the lock.

[1] https://docs.kernel.org/core-api/kref.html

Signed-off-by Chang Yu <[email protected]>
Reported-by [email protected]
Closes https://syzkaller.appspot.com/bug?extid=3e563d99e70973c0755c
Fixes f2c2e717642c ("usb: gadget: add raw-gadget interface")
---
 drivers/usb/gadget/legacy/raw_gadget.c | 44 ++++++++++++++------------
 1 file changed, 24 insertions(+), 20 deletions(-)
drivers/usb/gadget/legacy/raw_gadget.c +24 -20
index 112fd18d8c99..0c01d491d489 100644
@@ -225,7 +225,6 @@ static void dev_free(struct kref *kref)
 		kfree(dev->eps[i].ep->desc);
 		dev->eps[i].state = STATE_EP_DISABLED;
 	}
-	kfree(dev);
 }
 
 /*----------------------------------------------------------------------*/

@@ -330,7 +329,8 @@ static void gadget_unbind(struct usb_gadget *gadget)
 
 	set_gadget_data(gadget, NULL);
 	/* Matches kref_get() in gadget_bind(). */
-	kref_put(&dev->count, dev_free);
+	if (kref_put(&dev->count, dev_free))
+		kfree(dev);
 }
 
 static int gadget_setup(struct usb_gadget *gadget,

@@ -443,34 +443,38 @@ static int raw_open(struct inode *inode, struct file *fd)
 static int raw_release(struct inode *inode, struct file *fd)
 {
 	int ret = 0;
+	int freed = 0;
 	struct raw_dev *dev = fd->private_data;
 	unsigned long flags;
-	bool unregister = false;
 
 	spin_lock_irqsave(&dev->lock, flags);
 	dev->state = STATE_DEV_CLOSED;
-	if (!dev->gadget) {
-		spin_unlock_irqrestore(&dev->lock, flags);
-		goto out_put;
-	}
-	if (dev->gadget_registered)
-		unregister = true;
+	if (!dev->gadget)
+		goto out_put_locked;
+	if (!dev->gadget_registered)
+		goto out_put_locked;
 	dev->gadget_registered = false;
 	spin_unlock_irqrestore(&dev->lock, flags);
 
-	if (unregister) {
-		ret = usb_gadget_unregister_driver(&dev->driver);
-		if (ret != 0)
-			dev_err(dev->dev,
-				"usb_gadget_unregister_driver() failed with %d\n",
-				ret);
-		/* Matches kref_get() in raw_ioctl_run(). */
-		kref_put(&dev->count, dev_free);
-	}
+	ret = usb_gadget_unregister_driver(&dev->driver);
+	if (ret != 0)
+		dev_err(dev->dev,
+			"usb_gadget_unregister_driver() failed with %d\n",
+			ret);
+
+	spin_lock_irqsave(&dev->lock, flags);
+	/* Matches kref_get() in raw_ioctl_run(). */
+	freed = kref_put(&dev->count, dev_free);
+	if (freed)
+		goto out_free_dev;
 
-out_put:
+out_put_locked:
 	/* Matches dev_new() in raw_open(). */
-	kref_put(&dev->count, dev_free);
+	freed = kref_put(&dev->count, dev_free);
+out_free_dev:
+	spin_unlock_irqrestore(&dev->lock, flags);
+	if (freed)
+		kfree(dev);
 	return ret;
 }
Signature · 1 line
2.47.0
#1 lore raw
Quoted 6 lines · Chang Yu

syzkaller reported a double free bug
(https://syzkaller.appspot.com/bug?extid=3e563d99e70973c0755c) in raw_release.

I suspect this is because a race between raw_release and raw_ioctl_run.

This is confusing. raw_ioctl_run is called directly from raw_ioctl, which is invoked through an open file reference, so while it is running the file cannot be closed. But raw_release is called when the file is closed for the last time, so while it runs there cannot be any ioctls in progress.

Given this, how can the two routines race?

A debugging patch that shows what's going on whenever the kref is incremented or decremented might help clarify the situation.

Alan Stern

Quoted 10 lines

While kref_get in raw_ioctl_run is protected by
the spin lock, all kref_put in raw_release are not under the lock. This makes it possible that a kref_put might occur during kref_get, which is specifically prohibited by the kref documentation[1].

The fix is to ensure that all kref_put calls are made under lock and that we only call kfree(dev) after releasing the lock.

[1] https://docs.kernel.org/core-api/kref.html