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.

Cc: stable@vger.kernel.org
Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
Signed-off-by: Lovekesh Solanki <lovekeshsolanki00@gmail.com>
---
 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) {
 		++dev->udc_usage;
 		spin_unlock_irq(&dev->lock);
 
-- 
2.55.0

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@vger.kernel.org
> Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
> Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
> Signed-off-by: Lovekesh Solanki <lovekeshsolanki00@gmail.com>
> ---
>  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

Thanks for the review,

On Tue, Aug 25, 2026 at 09:14:37AM -0400, Alan Stern wrote:
> 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.
The order of reading it doesn't matter. The important part is to read it
while holding the lock, perhaps the wording is unclear, I'll reword it
in v2.

> 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.
Because ioctl can get a stale dev->gadget before dev->lock, while
dev->state is checked after acquiring the lock, which is the cause.
Is the reference to gadgetfs_bind() unncessary? Or this part of the
explanation is irrelvant?

> Why did you add this test for gadget being non-NULL?  Is there any way 
> it could possibly be NULL at this point?
It seems it doesn't matter since if read is correct it can't be NULL, it
was an initial attempt to fix but its unnecessary now, I'll remove that
as well.

Regards,
Lovekesh

On Tue, Aug 25, 2026 at 07:59:43PM +0530, Lovekesh Solanki wrote:
> Thanks for the review,
> 
> On Tue, Aug 25, 2026 at 09:14:37AM -0400, Alan Stern wrote:
> > 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.
> The order of reading it doesn't matter. The important part is to read it
> while holding the lock, perhaps the wording is unclear, I'll reword it
> in v2.
> 
> > 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.
> Because ioctl can get a stale dev->gadget before dev->lock, while
> dev->state is checked after acquiring the lock, which is the cause.

But those facts would remain true even if gadgetfs_bind() were to write 
dev->gadget while holding the lock, wouldn't they?  So the fact that the 
lock isn't held during the write makes no difference to your patch.

> Is the reference to gadgetfs_bind() unncessary? Or this part of the
> explanation is irrelvant?

Yes, it is irrelevant, for the reason just explained.

Alan Stern

> > Why did you add this test for gadget being non-NULL?  Is there any way 
> > it could possibly be NULL at this point?
> It seems it doesn't matter since if read is correct it can't be NULL, it
> was an initial attempt to fix but its unnecessary now, I'll remove that
> as well.
> 
> Regards,
> Lovekesh

On Tue, Aug 25, 2026 at 11:40:21AM -0400, Alan Stern wrote:
> But those facts would remain true even if gadgetfs_bind() were to write 
> dev->gadget while holding the lock, wouldn't they?  So the fact that the 
> lock isn't held during the write makes no difference to your patch.
I understand now, I've dropped these irrelavant parts from the commit
message and removed the NULL check in v2. 

Here is the link to v2:
https://lore.kernel.org/all/20260825171343.459630-1-lovekeshsolanki00@gmail.com/T/#u

Regards,
Lovekesh

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.
> 
> Cc: stable@vger.kernel.org
> Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
> Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
> Link: https://lore.kernel.org/all/20260824160022.2378192-1-jjy600901@snu.ac.kr/
> Signed-off-by: Lovekesh Solanki <lovekeshsolanki00@gmail.com>
> ---
>  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) {
>  		++dev->udc_usage;
>  		spin_unlock_irq(&dev->lock);
>  
> -- 
> 2.55.0

Apologies, the Link tag is wrong, correct one is
https://lore.kernel.org/all/20260824113510.1141236-1-jjy600901@snu.ac.kr/
Should I send a v2 with fixed tag?

Regards,
Lovekesh