usb: gadget: inode: fix use-after-free of struct ep_data

gadgetfs_unbind() -> destroy_ep_files() frees each struct ep_data via put_ep(), but the inode still points at that ep_data through ->i_private with no reference of its own. A concurrent openat() on an endpoint file can therefore reach ep_open() and dereference the freed ep_data (the mutex fast path), even while the inode outlives it.

Give the inode its own reference: take get_ep() when the endpoint inode is created and drop it again from a new gadgetfs_evict_inode(), so an ep_data can never be freed while any inode still points at it. This is also safe against dcache pruning, since the inode reference keeps the count >= 1 until the endpoint is removed from dev->epfiles.

Reported-by [email protected]

Link https://syzkaller.appspot.com/bug?extid=7bf725ed337e37307001

Tested-by [email protected]

Signed-off-by Adriano Cordova <[email protected]>

---

 drivers/usb/gadget/legacy/inode.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/usb/gadget/legacy/inode.c +13 -0

index db961aaa3740..089fa7119922 100644
@@ -1626,6 +1626,8 @@ static int activate_ep_files (struct dev_data *dev)
 		if (!data->req)
 			goto enomem1;
 
+		/* The inode keeps this ep_data alive via ->i_private. */
+		get_ep(data);
 		err = gadgetfs_create_file (dev->sb, data->name,
 				data, &ep_io_operations);
 		if (err)

@@ -2015,9 +2017,20 @@ static int gadgetfs_create_file (struct super_block *sb, char const *name,
 	return 0;
 }
 
+static void gadgetfs_evict_inode(struct inode *inode)
+{
+	/* EP inodes hold the reference on their ep_data via ->i_private. */
+	if (inode->i_fop == &ep_io_operations)
+		put_ep(inode->i_private);
+
+	truncate_inode_pages_final(&inode->i_data);
+	clear_inode(inode);
+}
+
 static const struct super_operations gadget_fs_operations = {
 	.statfs =	simple_statfs,
 	.drop_inode =	inode_just_drop,
+	.evict_inode =	gadgetfs_evict_inode,
 };
 
 static int

Signature · 1 line

2.51.0

Hi Alan,

Right. But that patch makes the inode the sole lifetime owner of the ep_data (deletes the list call to 'put_ep'), but ep_data is also linked in the list dev->epfiles and that reference can outlive the inode (it doesn't happen now I think, but only incidentally).

So in the patch in here I decided to go with a refcount approach, just to be safe.

Quoted 6 lines · Alan Stern

Quoted 5 lines · Adriano Cordova

gadgetfs_unbind() -> destroy_ep_files() frees each struct ep_data via put_ep(), but the inode still points at that ep_data through ->i_private with no reference of its own. A concurrent openat() on an endpoint file can therefore reach ep_open() and dereference the freed ep_data (the mutex fast path), even while the inode outlives it.

Quoted 5 lines

Quoted 5 lines

Give the inode its own reference: take get_ep() when the endpoint inode is created and drop it again from a new gadgetfs_evict_inode(), so an ep_data can never be freed while any inode still points at it. This is also safe against dcache pruning, since the inode reference keeps the count >= 1 until the endpoint is removed from dev->epfiles.

Quoted 6 lines · Adriano Córdova

Hi Alan,

Right. But that patch makes the inode the sole lifetime owner of the ep_data (deletes the list call to 'put_ep'), but ep_data is also linked in the list dev->epfiles and that reference can outlive the inode (it doesn't happen now I think, but only incidentally).

Okay, I see.

Quoted 2 lines

So in the patch in here I decided to go with a refcount approach, just to be safe.

The patch description could be improved just a little...

Quoted 9 lines

Quoted 6 lines · Alan Stern

Quoted 5 lines · Adriano Cordova

gadgetfs_unbind() -> destroy_ep_files() frees each struct ep_data via put_ep(), but the inode still points at that ep_data through ->i_private with no reference of its own. A concurrent openat() on an endpoint file can therefore reach ep_open() and dereference the freed ep_data (the mutex fast path), even while the inode outlives it.

You could mention something about the inode's lifetime and what causes it to be released (and when). This would make it clearer to people like me how a concurrent open() can race with inode destruction. But I agree, you're right that the inode's pointer should be counted in the ep_data's refcount.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Give the inode its own reference: take get_ep() when the endpoint inode is created and drop it again from a new gadgetfs_evict_inode(), so an ep_data can never be freed while any inode still points at it. This is also safe against dcache pruning, since the inode reference keeps the count >= 1 until the endpoint is removed from dev->epfiles.

Now it all makes sense.

Your patch doesn't handle the case where the gadgetfs_make_inode() call in gadgetfs_create_file() fails. If that happens, the refcount will remain elevated because there is no inode to evict. I'll leave it to you to figure out the best solution.

Alan Stern

Quoted 4 lines · Alan Stern

Your patch doesn't handle the case where the gadgetfs_make_inode() call in gadgetfs_create_file() fails. If that happens, the refcount will remain elevated because there is no inode to evict. I'll leave it to you to figure out the best solution.

If gadgetfs_make_inode() call in gadgetfs_create_file() fails, it returns -ENOMEM and the error path of activate_ep_files() (where the new inc to the refcount is) kfrees the object directly, bypassing the refcount.

I think the main CHIP has a dev_data instead of ep_data
and it belongs to the mount.