linux-kernel - Re: [syzbot] [PATCH] usb: raw_gadget: Fix a KASAN double-free bug in raw

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6726a11b.050a0220.35b515.0189.GAE@google.com>
Date: Sat, 02 Nov 2024 15:00:59 -0700
From: syzbot <syzbot+3e563d99e70973c0755c@...kaller.appspotmail.com>
To: linux-kernel@...r.kernel.org
Subject: Re: [syzbot] [PATCH] usb: raw_gadget: Fix a KASAN double-free bug in raw_release

For archival purposes, forwarding an incoming command email to
linux-kernel@...r.kernel.org.

***

Subject: [PATCH] usb: raw_gadget: Fix a KASAN double-free bug in raw_release
Author: marcus.yu.56@...il.com

#syz test

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@...il.com>
Reported-by: syzbot+3e563d99e70973c0755c@...kaller.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