Forwarded: [PATCH] usb: gadgetfs: fix use-after-free in ep_open()

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@xxxxxxxxxxxxxxx, syzkaller-bugs@xxxxxxxxxxxxxxxx.

***

Subject: [PATCH] usb: gadgetfs: fix use-after-free in ep_open()
Author: kartikey406@xxxxxxxxx

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-kernelci


ep_open() dereferenced inode->i_private (an ep_data pointer) and
locked its embedded mutex before ever validating that the object was
still alive:

    struct ep_data *data = inode->i_private;
    if (mutex_lock_interruptible(&data->lock) != 0)
            return -EINTR;
    spin_lock_irq(&data->dev->lock);
    if (data->dev->state == STATE_DEV_UNBOUND)
            ...

The liveness check (dev->state == STATE_DEV_UNBOUND) only happened
*after* data->lock had already been touched. destroy_ep_files(),
called from gadgetfs_unbind(), can free the same ep_data
concurrently via put_ep(), since nothing prevented an in-flight
open() from racing the teardown. KASAN reports a slab-use-after-free
read inside __mutex_lock_common() from ep_open(), with the object
freed by a concurrent gadgetfs_unbind() -> destroy_ep_files() ->
put_ep() -> kfree() on another task.

This exact race was identified by Al Viro on LKML in 2006 ("races in
drivers/usb/gadget/inode.c"), including the correct fix direction,
but it was never applied to ep_open() itself. Later fixes to this
file (the udc_usage counter, CVE-2022-4382) hardened the I/O paths
and the mount/unmount race, but left this specific ordering bug in
place.

Fix it by reordering ep_open() so that dev->state and inode->i_private
are only read while holding dev->lock -- reached via
inode->i_sb->s_fs_info, which stays valid for the life of the mount,
independent of any individual ep_data -- and by pinning the object
with get_ep() before dev->lock is dropped and before data->lock is
touched. This guarantees put_ep()/kfree() cannot race the mutex lock
in ep_open().

This fix depends on dev->state being set to STATE_DEV_UNBOUND under
dev->lock before any call to destroy_ep_files(), so that ep_open()
seeing a "bound" state is a guarantee that no free is in progress.
gadgetfs_unbind() already does this correctly. activate_ep_files()'s
enomem0 error-cleanup path did not: it called destroy_ep_files()
directly without first setting dev->state, which would have left the
same race open on the bind-failure path. This patch adds the missing
state transition there as well, so the invariant holds for every
caller of destroy_ep_files().

Reported-by: syzbot+df9e891bf8ea586f846b@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=df9e891bf8ea586f846b
Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
---
 drivers/usb/gadget/legacy/inode.c | 37 ++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index d87a8ab51510..95e3bcedd33d 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -817,25 +817,39 @@ ep_config (struct ep_data *data, const char *buf, size_t len)
 static int
 ep_open (struct inode *inode, struct file *fd)
 {
-	struct ep_data		*data = inode->i_private;
-	int			value = -EBUSY;
+	struct dev_data *dev = inode->i_sb->s_fs_info;
+	struct ep_data *data;
+	int value = -ENODEV;
+
+	spin_lock_irq (&dev->lock);
+	if (dev->state == STATE_DEV_UNBOUND) {
+		spin_unlock_irq (&dev->lock);
+		return -ENOENT;
+	}
+	data = inode->i_private;
+	get_ep (data);
+	spin_unlock_irq (&dev->lock);
 
-	if (mutex_lock_interruptible(&data->lock) != 0)
+	if (mutex_lock_interruptible(&data->lock) != 0) {
+		put_ep (data);
 		return -EINTR;
-	spin_lock_irq (&data->dev->lock);
-	if (data->dev->state == STATE_DEV_UNBOUND)
+	}
+
+	value = -EBUSY;
+	spin_lock_irq (&dev->lock);
+	if (dev->state == STATE_DEV_UNBOUND)
 		value = -ENOENT;
 	else if (data->state == STATE_EP_DISABLED) {
 		value = 0;
 		data->state = STATE_EP_READY;
-		get_ep (data);
 		fd->private_data = data;
-		VDEBUG (data->dev, "%s ready\n", data->name);
+		VDEBUG (dev, "%s ready\n", data->name);
 	} else
-		DBG (data->dev, "%s state %d\n",
-			data->name, data->state);
-	spin_unlock_irq (&data->dev->lock);
+		DBG (dev, "%s state %d\n", data->name, data->state);
+	spin_unlock_irq (&dev->lock);
 	mutex_unlock(&data->lock);
+	if (value)
+		put_ep (data);
 	return value;
 }
 
@@ -1632,6 +1646,9 @@ static int activate_ep_files (struct dev_data *dev)
 	kfree (data);
 enomem0:
 	DBG (dev, "%s enomem\n", __func__);
+	spin_lock_irq (&dev->lock);
+	dev->state = STATE_DEV_UNBOUND;
+	spin_unlock_irq (&dev->lock);
 	destroy_ep_files (dev);
 	return -ENOMEM;
 }
-- 
2.43.0