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 <marcus.yu.56@gmail.com>
Reported-by: syzbot+3e563d99e70973c0755c@syzkaller.appspotmail.com
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(-)

diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index 112fd18d8c99..0c01d491d489 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -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;
 }
 
-- 
2.47.0

On Sat, Nov 02, 2024 at 03:36:05PM -0700, Chang Yu wrote:
> 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

>  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