[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6721d497.050a0220.35b515.0001.GAE@google.com>
Date: Tue, 29 Oct 2024 23:39:19 -0700
From: syzbot <syzbot+3e563d99e70973c0755c@...kaller.appspotmail.com>
To: linux-kernel@...r.kernel.org
Subject: Re: [syzbot] [PATCH] usb: raw_gadget: Fix a KMSAN 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 KMSAN 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. Specifically, if there are concurrent calls to raw_release
and dev->gadget_registered is false, there will be racing kref_put
calls causing dev_free to be invoked twice.
The fix is to check ref count and put under lock so that kref_put is
called at most once.
The "unregister" path is safe because checking dev->gadget_registered
and then flipping it to false under lock guarantees that this branch is
taken at most once.
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 | 27 ++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index 112fd18d8c99..3da022aae830 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -445,7 +445,6 @@ static int raw_release(struct inode *inode, struct file *fd)
int ret = 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;
@@ -453,20 +452,24 @@ static int raw_release(struct inode *inode, struct file *fd)
spin_unlock_irqrestore(&dev->lock, flags);
goto out_put;
}
- if (dev->gadget_registered)
- unregister = true;
+ if (!dev->gadget_registered) {
+ if (kref_read(&dev->count)) {
+ /* Matches dev_new() in raw_open(). */
+ kref_put(&dev->count, dev_free);
+ }
+ spin_unlock_irqrestore(&dev->lock, flags);
+ return ret;
+ }
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);
+ /* Matches kref_get() in raw_ioctl_run(). */
+ kref_put(&dev->count, dev_free);
out_put:
/* Matches dev_new() in raw_open(). */
--
2.47.0
Powered by blists - more mailing lists