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

[PATCH] 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.

gadgetfs_create_file() gains an optional inode-out parameter so
activate_ep_files() can capture the inode it creates; the ep0 caller
in gadgetfs_fill_super() passes NULL, since dev_data is unaffected
by this fix.

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

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index d87a8ab51510..f566b2ec7c38 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 = the_device;
+	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 */
@@ -1588,7 +1605,8 @@ static void destroy_ep_files (struct dev_data *dev)
 
 
 static int gadgetfs_create_file (struct super_block *sb, char const *name,
-		void *data, const struct file_operations *fops);
+		void *data, const struct file_operations *fops,
+		struct inode **inode_out);
 
 static int activate_ep_files (struct dev_data *dev)
 {
@@ -1618,7 +1636,7 @@ static int activate_ep_files (struct dev_data *dev)
 			goto enomem1;
 
 		err = gadgetfs_create_file (dev->sb, data->name,
-				data, &ep_io_operations);
+				data, &ep_io_operations, &data->inode);
 		if (err)
 			goto enomem2;
 		list_add_tail (&data->epfiles, &dev->epfiles);
@@ -1663,8 +1681,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)
 {
@@ -1984,7 +2000,7 @@ gadgetfs_make_inode (struct super_block *sb,
  * so inode and dentry are paired, until device reconfig.
  */
 static int gadgetfs_create_file (struct super_block *sb, char const *name,
-		void *data, const struct file_operations *fops)
+		void *data, const struct file_operations *fops, struct inode **inode_out)
 {
 	struct dentry	*dentry;
 	struct inode	*inode;
@@ -1994,6 +2010,9 @@ static int gadgetfs_create_file (struct super_block *sb, char const *name,
 	if (!inode)
 		return -ENOMEM;
 
+	if (inode_out)
+		*inode_out = inode;
+
 	dentry = simple_start_creating(sb->s_root, name);
 	if (IS_ERR(dentry)) {
 		iput(inode);
@@ -2056,7 +2075,7 @@ 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);
+	rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations, NULL);
 	if (rc) {
 		put_dev(dev);
 		goto Enomem;
-- 
2.43.0

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

[Greg KH]

On Fri, Jul 31, 2026 at 02:43:16PM +0530, Deepanshu Kartikey wrote:
> 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.
> 
> gadgetfs_create_file() gains an optional inode-out parameter so
> activate_ep_files() can capture the inode it creates; the ep0 caller
> in gadgetfs_fill_super() passes NULL, since dev_data is unaffected
> by this fix.
> 
> Reported-by: syzbot+df9e891bf8ea586f846b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=df9e891bf8ea586f846b
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>

What commit id does this fix?

And how did you find this?  Test it?

> ---
>  drivers/usb/gadget/legacy/inode.c | 51 +++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index d87a8ab51510..f566b2ec7c38 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;

Do we really want a single "device"?  Ah, it comes from elsewhere in
thsi file, that's really not good...

And do you even use this code?  I ask as I'd really like to get rid of
all of the legacy gadget driver code.  I think Android was the last user
and they aren't using it anymore from what I can tell, but I could be
wrong...


>  
>  /*----------------------------------------------------------------------*/
>  
> @@ -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 = the_device;

Why isn't this coming from the inode or file?


> +	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 */
> @@ -1588,7 +1605,8 @@ static void destroy_ep_files (struct dev_data *dev)
>  
>  
>  static int gadgetfs_create_file (struct super_block *sb, char const *name,
> -		void *data, const struct file_operations *fops);
> +		void *data, const struct file_operations *fops,
> +		struct inode **inode_out);
>  
>  static int activate_ep_files (struct dev_data *dev)
>  {
> @@ -1618,7 +1636,7 @@ static int activate_ep_files (struct dev_data *dev)
>  			goto enomem1;
>  
>  		err = gadgetfs_create_file (dev->sb, data->name,
> -				data, &ep_io_operations);
> +				data, &ep_io_operations, &data->inode);
>  		if (err)
>  			goto enomem2;
>  		list_add_tail (&data->epfiles, &dev->epfiles);
> @@ -1663,8 +1681,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)
>  {
> @@ -1984,7 +2000,7 @@ gadgetfs_make_inode (struct super_block *sb,
>   * so inode and dentry are paired, until device reconfig.
>   */
>  static int gadgetfs_create_file (struct super_block *sb, char const *name,
> -		void *data, const struct file_operations *fops)
> +		void *data, const struct file_operations *fops, struct inode **inode_out)
>  {
>  	struct dentry	*dentry;
>  	struct inode	*inode;
> @@ -1994,6 +2010,9 @@ static int gadgetfs_create_file (struct super_block *sb, char const *name,
>  	if (!inode)
>  		return -ENOMEM;
>  
> +	if (inode_out)
> +		*inode_out = inode;
> +
>  	dentry = simple_start_creating(sb->s_root, name);
>  	if (IS_ERR(dentry)) {
>  		iput(inode);
> @@ -2056,7 +2075,7 @@ 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);
> +	rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations, NULL);

When we have to look up what a random parameter is, when you set it to
NULL, that's usually not a good way to write a function.

thanks,

gre k-h

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

[Deepanshu Kartikey]

On Fri, Jul 31, 2026 at 4:08 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>

> > Reported-by: syzbot+df9e891bf8ea586f846b@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=df9e891bf8ea586f846b
> > Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
>
> What commit id does this fix?
>

Fixes commit: I'll track down the exact SHA via git blame
on ep_open()'s lock ordering and resend with a proper Fixes: tag.


> And how did you find this?  Test it?
>

Found via syzbot fuzzing (KASAN slab-use-after-free),
not a manual audit reproduced locally under KASAN in a loop,
and confirmed via syzbot's #syz test against this patch.

> >
> >  static const char *CHIP;
> >  static DEFINE_MUTEX(sb_mutex);               /* Serialize superblock operations */
> > +static struct dev_data *the_device;
>
> Do we really want a single "device"?  Ah, it comes from elsewhere in
> thsi file, that's really not good...
>

I don't personally use gadgetfs found this purely from the syzbot report.
Happy to help check whether it's still in active use anywhere if that's useful,
or if you'd rather this go toward removing the legacy driver
 instead of patching it, that's fine by me too; your call.



> >  ep_open (struct inode *inode, struct file *fd)
> >  {
> > -     struct ep_data          *data = inode->i_private;
> > -     int                     value = -EBUSY;
> > +     struct dev_data *dev = the_device;
>
> Why isn't this coming from the inode or file?
>
>

Fair point; it doesn't need the_device at all.
I'll derive dev frominstead (setting sb->s_fs_info = dev in
gadgetfs_fill_super()),
which is properly scoped to this inode's mount rather than relying on
the global singleton.


> > -     rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations);
> > +     rc = gadgetfs_create_file(sb, CHIP, dev, &ep0_operations, NULL);
>
> When we have to look up what a random parameter is, when you set it to
> NULL, that's usually not a good way to write a function.
>

Correct, I will remove this no longer needed once I pick dev from
inode->i_sb->s_fs_info

I will send the patch v2 shortly

Thanks

Deepanshu