linux-kernel - [PATCH] usb: gadget: fix race when gadget driver register via ioctl

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-next>] [day] [month] [year] [list]

Message-Id: <20220507120851.29948-1-schspa@gmail.com>
Date:   Sat,  7 May 2022 20:08:51 +0800
From:   Schspa Shi <schspa@...il.com>
To:     andreyknvl@...il.com, balbi@...nel.org, gregkh@...uxfoundation.org
Cc:     jj251510319013@...il.com, stern@...land.harvard.edu,
        jannh@...gle.com, Julia.Lawall@...ia.fr, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, schspa@...il.com,
        syzbot+dc7c3ca638e773db07f6@...kaller.appspotmail.com
Subject: [PATCH] usb: gadget: fix race when gadget driver register via ioctl

The usb_gadget_register_driver doesn't have inside locks to protect the
driver, and If there is two threads are registered at the same time via
the ioctl syscall, the system will crash as syzbot reported.

Call trace as:
  driver_register+0x220/0x3a0 drivers/base/driver.c:171
  usb_gadget_register_driver_owner+0xfb/0x1e0
    drivers/usb/gadget/udc/core.c:1546
  raw_ioctl_run drivers/usb/gadget/legacy/raw_gadget.c:513 [inline]
  raw_ioctl+0x1883/0x2730 drivers/usb/gadget/legacy/raw_gadget.c:1220

This routine allows two processes to register the same driver instance
via ioctl syscall. which lead to a race condition.

We can fix it by adding a driver_lock to avoid double register.

Reported-by: syzbot+dc7c3ca638e773db07f6@...kaller.appspotmail.com
Link: https://lore.kernel.org/all/000000000000e66c2805de55b15a@google.com/

Signed-off-by: Schspa Shi <schspa@...il.com>
---
 drivers/usb/gadget/legacy/raw_gadget.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
index b3be8db1ff63..d7ff9c2b5397 100644
--- a/drivers/usb/gadget/legacy/raw_gadget.c
+++ b/drivers/usb/gadget/legacy/raw_gadget.c
@@ -155,7 +155,9 @@ struct raw_dev {
 	spinlock_t			lock;
 
 	const char			*udc_name;
+	/* Protected by driver_lock for reentrant registration */
 	struct usb_gadget_driver	driver;
+	struct mutex			driver_lock;
 
 	/* Reference to misc device: */
 	struct device			*dev;
@@ -188,6 +190,8 @@ static struct raw_dev *dev_new(void)
 	spin_lock_init(&dev->lock);
 	init_completion(&dev->ep0_done);
 	raw_event_queue_init(&dev->queue);
+	mutex_init(&dev->driver_lock);
+
 	return dev;
 }
 
@@ -398,7 +402,9 @@ static int raw_release(struct inode *inode, struct file *fd)
 	spin_unlock_irqrestore(&dev->lock, flags);
 
 	if (unregister) {
+		mutex_lock(&dev->driver_lock);
 		ret = usb_gadget_unregister_driver(&dev->driver);
+		mutex_unlock(&dev->driver_lock);
 		if (ret != 0)
 			dev_err(dev->dev,
 				"usb_gadget_unregister_driver() failed with %d\n",
@@ -510,7 +516,9 @@ static int raw_ioctl_run(struct raw_dev *dev, unsigned long value)
 	}
 	spin_unlock_irqrestore(&dev->lock, flags);
 
+	mutex_lock(&dev->driver_lock);
 	ret = usb_gadget_register_driver(&dev->driver);
+	mutex_unlock(&dev->driver_lock);
 
 	spin_lock_irqsave(&dev->lock, flags);
 	if (ret) {
-- 
2.29.0