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

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

[Deepanshu Kartikey]

ep_open() dereferenced inode->i_private and locked its embedded
mutex before checking whether the underlying ep_data was still
alive. destroy_ep_files(), called from gadgetfs_unbind(), can free
that same ep_data concurrently, since nothing prevented an in-flight
open() from racing the teardown. KASAN reports a slab-use-after-free
in ep_open(), with the object freed by a concurrent gadgetfs_unbind()
-> destroy_ep_files() -> put_ep() -> kfree().

Fix it by adding an inode back-pointer to ep_data, having
destroy_ep_files() clear inode->i_private to NULL under dev->lock
before the object can be freed, and having ep_open() read
inode->i_private and pin the result with get_ep() under that same
lock, bailing out with -ENOENT if it finds NULL. This makes liveness
self-evident at the point of use instead of being inferred from
device state, so it holds regardless of which path frees the
endpoint.

dev is derived from inode->i_sb->s_fs_info, set in
gadgetfs_fill_super(), rather than from the file's the_device
singleton, so ep_open() is correctly scoped to the specific mount
the inode belongs to.

gadgetfs_create_file() now returns the created inode (or an
ERR_PTR) directly instead of taking an output parameter, so
activate_ep_files() can store it in data->inode without an
unexplained NULL at call sites that don't need it.

Reported-by: syzbot+df9e891bf8ea586f846b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=df9e891bf8ea586f846b
Fixes: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 ("Linux-2.6.12-rc2")
Tested-by: syzbot+df9e891bf8ea586f846b@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
v2:
 - Added Fixes: tag; git blame traces the bug to the 2005 git
   import boundary, confirmed no later commit changed the ordering
 - Derive dev from inode->i_sb->s_fs_info instead of the new
   the_device singleton, so the lookup is scoped to the inode's
   own mount instead of a global (Greg)
 - gadgetfs_create_file() now returns struct inode * / ERR_PTR
   instead of taking an inode_out output parameter, so call sites
   don't need an unexplained NULL argument (Greg)
---
 drivers/usb/gadget/legacy/inode.c | 61 ++++++++++++++++++++-----------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index d87a8ab51510..e5a316777db2 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -207,6 +207,7 @@ struct ep_data {
 	struct usb_endpoint_descriptor	desc, hs_desc;
 	struct list_head		epfiles;
 	wait_queue_head_t		wait;
+	struct inode                    *inode;
 };
 
 static inline void get_ep (struct ep_data *data)
@@ -235,6 +236,7 @@ static void put_ep (struct ep_data *data)
 
 static const char *CHIP;
 static DEFINE_MUTEX(sb_mutex);		/* Serialize superblock operations */
+static struct dev_data *the_device;
 
 /*----------------------------------------------------------------------*/
 
@@ -817,25 +819,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);
+	data = inode->i_private;
+	if (data == NULL) {
+		spin_unlock_irq(&dev->lock);
+		return -ENOENT;
+	}
+	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;
 }
 
@@ -1563,6 +1579,7 @@ static void destroy_ep_files (struct dev_data *dev)
 		/* break link to FS */
 		ep = list_first_entry (&dev->epfiles, struct ep_data, epfiles);
 		list_del_init (&ep->epfiles);
+		ep->inode->i_private = NULL;
 		spin_unlock_irq (&dev->lock);
 
 		/* break link to controller */
@@ -1587,14 +1604,13 @@ static void destroy_ep_files (struct dev_data *dev)
 }
 
 
-static int gadgetfs_create_file (struct super_block *sb, char const *name,
+static struct inode *gadgetfs_create_file(struct super_block *sb, char const *name,
 		void *data, const struct file_operations *fops);
 
 static int activate_ep_files (struct dev_data *dev)
 {
 	struct usb_ep	*ep;
 	struct ep_data	*data;
-	int err;
 
 	gadget_for_each_ep (ep, dev->gadget) {
 
@@ -1617,9 +1633,9 @@ static int activate_ep_files (struct dev_data *dev)
 		if (!data->req)
 			goto enomem1;
 
-		err = gadgetfs_create_file (dev->sb, data->name,
+		data->inode = gadgetfs_create_file(dev->sb, data->name,
 				data, &ep_io_operations);
-		if (err)
+		if (IS_ERR(data->inode))
 			goto enomem2;
 		list_add_tail (&data->epfiles, &dev->epfiles);
 	}
@@ -1663,8 +1679,6 @@ gadgetfs_unbind (struct usb_gadget *gadget)
 	put_dev (dev);
 }
 
-static struct dev_data		*the_device;
-
 static int gadgetfs_bind(struct usb_gadget *gadget,
 		struct usb_gadget_driver *driver)
 {
@@ -1983,7 +1997,7 @@ gadgetfs_make_inode (struct super_block *sb,
 /* creates in fs root directory, so non-renamable and non-linkable.
  * so inode and dentry are paired, until device reconfig.
  */
-static int gadgetfs_create_file (struct super_block *sb, char const *name,
+static struct inode *gadgetfs_create_file(struct super_block *sb, char const *name,
 		void *data, const struct file_operations *fops)
 {
 	struct dentry	*dentry;
@@ -1992,18 +2006,18 @@ static int gadgetfs_create_file (struct super_block *sb, char const *name,
 	inode = gadgetfs_make_inode (sb, data, fops,
 			S_IFREG | (default_perm & S_IRWXUGO));
 	if (!inode)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	dentry = simple_start_creating(sb->s_root, name);
 	if (IS_ERR(dentry)) {
 		iput(inode);
-		return PTR_ERR(dentry);
+		return ERR_CAST(dentry);
 	}
 
 	d_make_persistent(dentry, inode);
 
 	simple_done_creating(dentry);
-	return 0;
+	return inode;
 }
 
 static const struct super_operations gadget_fs_operations = {
@@ -2056,8 +2070,11 @@ gadgetfs_fill_super (struct super_block *sb, struct fs_context *fc)
 		goto Enomem;
 
 	dev->sb = sb;
-	rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
-	if (rc) {
+	sb->s_fs_info = dev;
+	inode = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
+	if (IS_ERR(inode)) {
+		rc = PTR_ERR(inode);
+		sb->s_fs_info = NULL;
 		put_dev(dev);
 		goto Enomem;
 	}
-- 
2.43.0