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

[Greg KH <gregkh@linuxfoundation.org>]

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