Re: [PATCH] USB: gadget: fix NULL pointer dereference in gadget_dev_ioctl()

[Alan Stern]

On Tue, Aug 25, 2026 at 04:28:01PM +0530, Lovekesh Solanki wrote:
> gadget_dev_ioctl() reads dev->gadget outside the dev->lock, while
> gadgetfs_bind() writes it without holding the lock. A concurrent
> bind can update dev->gadget and dev->state under the lock while the
> ioctl thread holds a stale NULL copy, causing a NULL pointer
> dereference at offset 0x28 (gadget->ops->ioctl).
> 
> Read dev->gadget inside the locked region, before the state check,
> so the state and gadget pointer are always consistent.

Why does it matter that you read dev->gadget before the state check 
rather than after?  If it doesn't matter, there's no reason to mention 
it in the patch description.

Also, why does it matter that gadgetfs_bind() writes dev->gadget without 
holding the lock?  Again, the description shouldn't mention things that 
don't matter.

> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Eulgyu Kim <eulgyukim@xxxxxxxxx>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@xxxxxxxxx/
> Reported-by: Jaeyoung Chung <jjy600901@xxxxxxxxx>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@xxxxxxxxx/
> Signed-off-by: Lovekesh Solanki <lovekeshsolanki00@xxxxxxxxx>
> ---
>  drivers/usb/gadget/legacy/inode.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index d87a8ab51510..e9f7d7c1a6a3 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
> @@ -1251,14 +1251,15 @@ ep0_poll (struct file *fd, poll_table *wait)
>  static long gadget_dev_ioctl (struct file *fd, unsigned code, unsigned long value)
>  {
>  	struct dev_data		*dev = fd->private_data;
> -	struct usb_gadget	*gadget = dev->gadget;
> +	struct usb_gadget	*gadget;
>  	long ret = -ENOTTY;
>  
>  	spin_lock_irq(&dev->lock);
> +	gadget = dev->gadget;
>  	if (dev->state == STATE_DEV_OPENED ||
>  			dev->state == STATE_DEV_UNBOUND) {
>  		/* Not bound to a UDC */
> -	} else if (gadget->ops->ioctl) {
> +	} else if (gadget && gadget->ops->ioctl) {

Why did you add this test for gadget being non-NULL?  Is there any way 
it could possibly be NULL at this point?

Alan Stern

>  		++dev->udc_usage;
>  		spin_unlock_irq(&dev->lock);
>  
> -- 
> 2.55.0