[PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Selvarasu Ganesan]

This commit adds a null pointer check for udc in gadget_match_driver to
prevent the below potential dangling pointer access. The issue arises
due to continuous USB role switch and simultaneous UDC write operations
performed by init.rc from user space through configfs.  In these
scenarios, there was a possibility of usb_udc_release being done before
gadget_match_driver.

[27635.233849]  BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
[27635.233871]  Read of size 8 at addr d7ffff8837ead080 by task init/1
[27635.233881]  Pointer tag: [d7], memory tag: [fe]
[27635.233888]
[27635.233917]  Call trace:
[27635.233923]   dump_backtrace+0xec/0x10c
[27635.233935]   show_stack+0x18/0x24
[27635.233944]   dump_stack_lvl+0x50/0x6c
[27635.233958]   print_report+0x150/0x6b4
[27635.233977]   kasan_report+0xe8/0x148
[27635.233985]   __hwasan_load8_noabort+0x88/0x98
[27635.233995]   gadget_match_driver+0x40/0x94
[27635.234005]   __driver_attach+0x60/0x304
[27635.234018]   bus_for_each_dev+0x154/0x1b4
[27635.234027]   driver_attach+0x34/0x48
[27635.234036]   bus_add_driver+0x1ec/0x310
[27635.234045]   driver_register+0xc8/0x1b4
[27635.234055]   usb_gadget_register_driver_owner+0x7c/0x140
[27635.234066]   gadget_dev_desc_UDC_store+0x148/0x19c
[27635.234075]   configfs_write_iter+0x180/0x1e0
[27635.234087]   vfs_write+0x298/0x3e4
[27635.234105]   ksys_write+0x88/0x100
[27635.234115]   __arm64_sys_write+0x44/0x5c
[27635.234126]   invoke_syscall+0x6c/0x17c
[27635.234143]   el0_svc_common+0xf8/0x138
[27635.234154]   do_el0_svc+0x30/0x40
[27635.234164]   el0_svc+0x38/0x68
[27635.234174]   el0t_64_sync_handler+0x68/0xbc
[27635.234184]   el0t_64_sync+0x19c/0x1a0

Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
Cc: stable <stable@kernel.org>
Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
---
 drivers/usb/gadget/udc/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index cf6478f97f4a..77dc0f28ff01 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
 	udc = container_of(dev, struct usb_udc, dev);
 	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
 	kfree(udc);
+	udc = NULL;
 }
 
 static const struct attribute_group *usb_udc_attr_groups[];
@@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
 			struct usb_gadget_driver, driver);
 
 	/* If the driver specifies a udc_name, it must match the UDC's name */
-	if (driver->udc_name &&
+	if (driver->udc_name && udc &&
 			strcmp(driver->udc_name, dev_name(&udc->dev)) != 0)
 		return 0;
 
-- 
2.17.1

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Greg KH]

On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
> This commit adds a null pointer check for udc in gadget_match_driver to
> prevent the below potential dangling pointer access. The issue arises
> due to continuous USB role switch and simultaneous UDC write operations
> performed by init.rc from user space through configfs.  In these
> scenarios, there was a possibility of usb_udc_release being done before
> gadget_match_driver.
> 
> [27635.233849]  BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
> [27635.233871]  Read of size 8 at addr d7ffff8837ead080 by task init/1
> [27635.233881]  Pointer tag: [d7], memory tag: [fe]
> [27635.233888]
> [27635.233917]  Call trace:
> [27635.233923]   dump_backtrace+0xec/0x10c
> [27635.233935]   show_stack+0x18/0x24
> [27635.233944]   dump_stack_lvl+0x50/0x6c
> [27635.233958]   print_report+0x150/0x6b4
> [27635.233977]   kasan_report+0xe8/0x148
> [27635.233985]   __hwasan_load8_noabort+0x88/0x98
> [27635.233995]   gadget_match_driver+0x40/0x94
> [27635.234005]   __driver_attach+0x60/0x304
> [27635.234018]   bus_for_each_dev+0x154/0x1b4
> [27635.234027]   driver_attach+0x34/0x48
> [27635.234036]   bus_add_driver+0x1ec/0x310
> [27635.234045]   driver_register+0xc8/0x1b4
> [27635.234055]   usb_gadget_register_driver_owner+0x7c/0x140
> [27635.234066]   gadget_dev_desc_UDC_store+0x148/0x19c
> [27635.234075]   configfs_write_iter+0x180/0x1e0
> [27635.234087]   vfs_write+0x298/0x3e4
> [27635.234105]   ksys_write+0x88/0x100
> [27635.234115]   __arm64_sys_write+0x44/0x5c
> [27635.234126]   invoke_syscall+0x6c/0x17c
> [27635.234143]   el0_svc_common+0xf8/0x138
> [27635.234154]   do_el0_svc+0x30/0x40
> [27635.234164]   el0_svc+0x38/0x68
> [27635.234174]   el0t_64_sync_handler+0x68/0xbc
> [27635.234184]   el0t_64_sync+0x19c/0x1a0
> 
> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> Cc: stable <stable@kernel.org>
> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> ---
>  drivers/usb/gadget/udc/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index cf6478f97f4a..77dc0f28ff01 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
>  	udc = container_of(dev, struct usb_udc, dev);
>  	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>  	kfree(udc);
> +	udc = NULL;

That's not ok, as what happens if you race right between freeing it and
accessing it elsewhere?

>  }
>  
>  static const struct attribute_group *usb_udc_attr_groups[];
> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
>  			struct usb_gadget_driver, driver);
>  
>  	/* If the driver specifies a udc_name, it must match the UDC's name */
> -	if (driver->udc_name &&
> +	if (driver->udc_name && udc &&

I agree this isn't good, but you just made the window smaller, please
fix this properly.

thanks,

greg k-h

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Selvarasu Ganesan]

On 8/28/2024 3:09 PM, Greg KH wrote:
> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
>> This commit adds a null pointer check for udc in gadget_match_driver to
>> prevent the below potential dangling pointer access. The issue arises
>> due to continuous USB role switch and simultaneous UDC write operations
>> performed by init.rc from user space through configfs.  In these
>> scenarios, there was a possibility of usb_udc_release being done before
>> gadget_match_driver.
>>
>> [27635.233849]  BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
>> [27635.233871]  Read of size 8 at addr d7ffff8837ead080 by task init/1
>> [27635.233881]  Pointer tag: [d7], memory tag: [fe]
>> [27635.233888]
>> [27635.233917]  Call trace:
>> [27635.233923]   dump_backtrace+0xec/0x10c
>> [27635.233935]   show_stack+0x18/0x24
>> [27635.233944]   dump_stack_lvl+0x50/0x6c
>> [27635.233958]   print_report+0x150/0x6b4
>> [27635.233977]   kasan_report+0xe8/0x148
>> [27635.233985]   __hwasan_load8_noabort+0x88/0x98
>> [27635.233995]   gadget_match_driver+0x40/0x94
>> [27635.234005]   __driver_attach+0x60/0x304
>> [27635.234018]   bus_for_each_dev+0x154/0x1b4
>> [27635.234027]   driver_attach+0x34/0x48
>> [27635.234036]   bus_add_driver+0x1ec/0x310
>> [27635.234045]   driver_register+0xc8/0x1b4
>> [27635.234055]   usb_gadget_register_driver_owner+0x7c/0x140
>> [27635.234066]   gadget_dev_desc_UDC_store+0x148/0x19c
>> [27635.234075]   configfs_write_iter+0x180/0x1e0
>> [27635.234087]   vfs_write+0x298/0x3e4
>> [27635.234105]   ksys_write+0x88/0x100
>> [27635.234115]   __arm64_sys_write+0x44/0x5c
>> [27635.234126]   invoke_syscall+0x6c/0x17c
>> [27635.234143]   el0_svc_common+0xf8/0x138
>> [27635.234154]   do_el0_svc+0x30/0x40
>> [27635.234164]   el0_svc+0x38/0x68
>> [27635.234174]   el0t_64_sync_handler+0x68/0xbc
>> [27635.234184]   el0t_64_sync+0x19c/0x1a0
>>
>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>> Cc: stable <stable@kernel.org>
>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
>> ---
>>   drivers/usb/gadget/udc/core.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index cf6478f97f4a..77dc0f28ff01 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
>>   	udc = container_of(dev, struct usb_udc, dev);
>>   	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>>   	kfree(udc);
>> +	udc = NULL;
> That's not ok, as what happens if you race right between freeing it and
> accessing it elsewhere?

Hi Greg,

Thanks for your comments.
Agree This race can occur at any time, and we are investigating the 
possibility of this issue through the following call trace. In our 
entire test sequence, the only place where we encounter UDC null is in 
the gadget_match_driver. It seems difficult to use locking to prevent 
this race, so we believe it would be acceptable to implement a NULL 
pointer check. We would appreciate any alternative solutions you may 
suggest.


CPU0: (ROLE SWITCH DEVICE -> HOST) CPU1 (echo "<dwc3 device name>" > UDC)
============================================================================== 

VENDOR usb notify driver()
VENDOR USB glue driver()              configfs_write_iter()
    usb_role_switch_set_role() gadget_dev_desc_UDC_store()
      dwc3_usb_role_switch_set()           driver_register()
        dwc3_set_mode()                     bus_add_driver()
         __dwc3_set_mode()                    driver_attach()
            dwc3_gadget_exit()                  bus_for_each_dev()
             usb_put_gadget(dwc->gadget); __driver_attach()
               usb_udc_release()
gadget_match_driver()
>
>>   }
>>   
>>   static const struct attribute_group *usb_udc_attr_groups[];
>> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
>>   			struct usb_gadget_driver, driver);
>>   
>>   	/* If the driver specifies a udc_name, it must match the UDC's name */
>> -	if (driver->udc_name &&
>> +	if (driver->udc_name && udc &&
> I agree this isn't good, but you just made the window smaller, please
> fix this properly.
>
> thanks,
>
> greg k-h


Sorry i did not understand on the above statement. Could you please 
provide more details on this?. Please correct me if i am wrong, Based on 
your statement, it seems that the time between the role switch and the 
write UDC is shorter than what is required, and you believe that we need 
to fix our glue driver itself where trigger the 
usb_role_switch_set_role(). Is that correct understanding?

Thanks,
Selva
>

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Alan Stern]

On Wed, Aug 28, 2024 at 11:39:58AM +0200, Greg KH wrote:
> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
> > This commit adds a null pointer check for udc in gadget_match_driver to
> > prevent the below potential dangling pointer access. The issue arises
> > due to continuous USB role switch and simultaneous UDC write operations
> > performed by init.rc from user space through configfs.  In these
> > scenarios, there was a possibility of usb_udc_release being done before
> > gadget_match_driver.
> > 
> > [27635.233849]  BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
> > [27635.233871]  Read of size 8 at addr d7ffff8837ead080 by task init/1
> > [27635.233881]  Pointer tag: [d7], memory tag: [fe]
> > [27635.233888]
> > [27635.233917]  Call trace:
> > [27635.233923]   dump_backtrace+0xec/0x10c
> > [27635.233935]   show_stack+0x18/0x24
> > [27635.233944]   dump_stack_lvl+0x50/0x6c
> > [27635.233958]   print_report+0x150/0x6b4
> > [27635.233977]   kasan_report+0xe8/0x148
> > [27635.233985]   __hwasan_load8_noabort+0x88/0x98
> > [27635.233995]   gadget_match_driver+0x40/0x94
> > [27635.234005]   __driver_attach+0x60/0x304
> > [27635.234018]   bus_for_each_dev+0x154/0x1b4
> > [27635.234027]   driver_attach+0x34/0x48
> > [27635.234036]   bus_add_driver+0x1ec/0x310
> > [27635.234045]   driver_register+0xc8/0x1b4
> > [27635.234055]   usb_gadget_register_driver_owner+0x7c/0x140
> > [27635.234066]   gadget_dev_desc_UDC_store+0x148/0x19c
> > [27635.234075]   configfs_write_iter+0x180/0x1e0
> > [27635.234087]   vfs_write+0x298/0x3e4
> > [27635.234105]   ksys_write+0x88/0x100
> > [27635.234115]   __arm64_sys_write+0x44/0x5c
> > [27635.234126]   invoke_syscall+0x6c/0x17c
> > [27635.234143]   el0_svc_common+0xf8/0x138
> > [27635.234154]   do_el0_svc+0x30/0x40
> > [27635.234164]   el0_svc+0x38/0x68
> > [27635.234174]   el0t_64_sync_handler+0x68/0xbc
> > [27635.234184]   el0t_64_sync+0x19c/0x1a0
> > 
> > Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
> > Cc: stable <stable@kernel.org>
> > Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
> > ---
> >  drivers/usb/gadget/udc/core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> > index cf6478f97f4a..77dc0f28ff01 100644
> > --- a/drivers/usb/gadget/udc/core.c
> > +++ b/drivers/usb/gadget/udc/core.c
> > @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
> >  	udc = container_of(dev, struct usb_udc, dev);
> >  	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
> >  	kfree(udc);
> > +	udc = NULL;
> 
> That's not ok, as what happens if you race right between freeing it and
> accessing it elsewhere?

In fact, this assignment does nothing at all.  This is at the end of the 
function and udc is a local variable, so it's not going to be used 
again.  The compiler won't even generate any code for this.

> >  }
> >  
> >  static const struct attribute_group *usb_udc_attr_groups[];
> > @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
> >  			struct usb_gadget_driver, driver);
> >  
> >  	/* If the driver specifies a udc_name, it must match the UDC's name */
> > -	if (driver->udc_name &&
> > +	if (driver->udc_name && udc &&
> 
> I agree this isn't good, but you just made the window smaller, please
> fix this properly.

I don't see how udc can possibly be NULL here.  It gets initialized to a 
non-NULL value when usb_add_gadget() does:

	gadget->udc = udc;

and nothing changes its value thereafter.  It seems much more likely 
that the error shown above is an invalid pointer access because 
gadget->udc points to a location that has been deallocated.  Adding this 
NULL check won't fix the bug.

Apparently the problem is caused by the fact that bus_for_each_dev(), 
iterating over the things on the gadget bus, is still using gadget after

	device_del(&gadget->dev);

in usb_del_gadget() returns and while

	device_unregister(&udc->dev);

runs and the udc structure is deallocated.  The only solution I can 
think of is for the gadget to take a reference to the udc and drop the 
reference when the gadget is released.  Unfortunately, several UDC 
drivers define their own gadget-release routines; they will all need to 
be modified.  And the core will need its own gadget-release routine for 
use when the UDC driver does not specify its own.

Alan Stern

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Selvarasu Ganesan]

On 8/28/2024 8:24 PM, Alan Stern wrote:
> On Wed, Aug 28, 2024 at 11:39:58AM +0200, Greg KH wrote:
>> On Wed, Aug 28, 2024 at 12:35:04PM +0530, Selvarasu Ganesan wrote:
>>> This commit adds a null pointer check for udc in gadget_match_driver to
>>> prevent the below potential dangling pointer access. The issue arises
>>> due to continuous USB role switch and simultaneous UDC write operations
>>> performed by init.rc from user space through configfs.  In these
>>> scenarios, there was a possibility of usb_udc_release being done before
>>> gadget_match_driver.
>>>
>>> [27635.233849]  BUG: KASAN: invalid-access in gadget_match_driver+0x40/0x94
>>> [27635.233871]  Read of size 8 at addr d7ffff8837ead080 by task init/1
>>> [27635.233881]  Pointer tag: [d7], memory tag: [fe]
>>> [27635.233888]
>>> [27635.233917]  Call trace:
>>> [27635.233923]   dump_backtrace+0xec/0x10c
>>> [27635.233935]   show_stack+0x18/0x24
>>> [27635.233944]   dump_stack_lvl+0x50/0x6c
>>> [27635.233958]   print_report+0x150/0x6b4
>>> [27635.233977]   kasan_report+0xe8/0x148
>>> [27635.233985]   __hwasan_load8_noabort+0x88/0x98
>>> [27635.233995]   gadget_match_driver+0x40/0x94
>>> [27635.234005]   __driver_attach+0x60/0x304
>>> [27635.234018]   bus_for_each_dev+0x154/0x1b4
>>> [27635.234027]   driver_attach+0x34/0x48
>>> [27635.234036]   bus_add_driver+0x1ec/0x310
>>> [27635.234045]   driver_register+0xc8/0x1b4
>>> [27635.234055]   usb_gadget_register_driver_owner+0x7c/0x140
>>> [27635.234066]   gadget_dev_desc_UDC_store+0x148/0x19c
>>> [27635.234075]   configfs_write_iter+0x180/0x1e0
>>> [27635.234087]   vfs_write+0x298/0x3e4
>>> [27635.234105]   ksys_write+0x88/0x100
>>> [27635.234115]   __arm64_sys_write+0x44/0x5c
>>> [27635.234126]   invoke_syscall+0x6c/0x17c
>>> [27635.234143]   el0_svc_common+0xf8/0x138
>>> [27635.234154]   do_el0_svc+0x30/0x40
>>> [27635.234164]   el0_svc+0x38/0x68
>>> [27635.234174]   el0t_64_sync_handler+0x68/0xbc
>>> [27635.234184]   el0t_64_sync+0x19c/0x1a0
>>>
>>> Fixes: fc274c1e9973 ("USB: gadget: Add a new bus for gadgets")
>>> Cc: stable <stable@kernel.org>
>>> Signed-off-by: Selvarasu Ganesan <selvarasu.g@samsung.com>
>>> ---
>>>   drivers/usb/gadget/udc/core.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>>> index cf6478f97f4a..77dc0f28ff01 100644
>>> --- a/drivers/usb/gadget/udc/core.c
>>> +++ b/drivers/usb/gadget/udc/core.c
>>> @@ -1338,6 +1338,7 @@ static void usb_udc_release(struct device *dev)
>>>   	udc = container_of(dev, struct usb_udc, dev);
>>>   	dev_dbg(dev, "releasing '%s'\n", dev_name(dev));
>>>   	kfree(udc);
>>> +	udc = NULL;
>> That's not ok, as what happens if you race right between freeing it and
>> accessing it elsewhere?
> In fact, this assignment does nothing at all.  This is at the end of the
> function and udc is a local variable, so it's not going to be used
> again.  The compiler won't even generate any code for this.
>
>>>   }
>>>   
>>>   static const struct attribute_group *usb_udc_attr_groups[];
>>> @@ -1574,7 +1575,7 @@ static int gadget_match_driver(struct device *dev, const struct device_driver *d
>>>   			struct usb_gadget_driver, driver);
>>>   
>>>   	/* If the driver specifies a udc_name, it must match the UDC's name */
>>> -	if (driver->udc_name &&
>>> +	if (driver->udc_name && udc &&
>> I agree this isn't good, but you just made the window smaller, please
>> fix this properly.
> I don't see how udc can possibly be NULL here.  It gets initialized to a
> non-NULL value when usb_add_gadget() does:
>
> 	gadget->udc = udc;
>
> and nothing changes its value thereafter.  It seems much more likely
> that the error shown above is an invalid pointer access because
> gadget->udc points to a location that has been deallocated.  Adding this
> NULL check won't fix the bug.
>
> Apparently the problem is caused by the fact that bus_for_each_dev(),
> iterating over the things on the gadget bus, is still using gadget after
>
> 	device_del(&gadget->dev);
>
> in usb_del_gadget() returns and while
>
> 	device_unregister(&udc->dev);
>
> runs and the udc structure is deallocated.  The only solution I can
> think of is for the gadget to take a reference to the udc and drop the
> reference when the gadget is released.  Unfortunately, several UDC
> drivers define their own gadget-release routines; they will all need to
> be modified.  And the core will need its own gadget-release routine for
> use when the UDC driver does not specify its own.
>
> Alan Stern
Hi Alan,

Thanks for your comments. I understand your suggestions. We already have 
a similar reference check with the udc name before calling 
usb_gadget_register_driver.
In the drivers/usb/gadget/configfs.c file, I am wondering if there might 
be an issue with the check of udc_name before 
usb_gadget_register_driver. This is the only way to allow 
gadget_register to be called before releasing or unregistering an 
existing udc. Do you think we need to add an additional check here, 
referencing the UDC, to prevent gadget_register from being called before 
the existing UDC is released?



drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
===========================================================
if (gi->composite.gadget_driver.udc_name) {
                         ret = -EBUSY;
                         goto err;
                 }
gi->composite.gadget_driver.udc_name = name;
ret = usb_gadget_register_driver(&gi->composite.gadget_driver);


Thanks,
Selva
>

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Alan Stern]

On Fri, Aug 30, 2024 at 06:16:12PM +0530, Selvarasu Ganesan wrote:
> Hi Alan,
> 
> Thanks for your comments. I understand your suggestions. We already have 
> a similar reference check with the udc name before calling 
> usb_gadget_register_driver.
> In the drivers/usb/gadget/configfs.c file, I am wondering if there might 
> be an issue with the check of udc_name before 
> usb_gadget_register_driver. This is the only way to allow 
> gadget_register to be called before releasing or unregistering an 
> existing udc. Do you think we need to add an additional check here, 
> referencing the UDC, to prevent gadget_register from being called before 
> the existing UDC is released?

I don't understand what you're saying.  There is no routine named 
"gadget_register".  (And there is no variable named "udc_name" in the 
code below, although there is gi->composite.gadget_driver.udc_name -- 
but that's not a variable, it is a field in a structure.)

> drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
> ===========================================================
> if (gi->composite.gadget_driver.udc_name) {
>                          ret = -EBUSY;
>                          goto err;
>                  }
> gi->composite.gadget_driver.udc_name = name;

Are you talking about this check and assignment?  Why do you think there 
might be a problem here?

Are you worried that some UDC might be released while this code is 
running?  If that happens, why would it be a problem?

> ret = usb_gadget_register_driver(&gi->composite.gadget_driver);

Alan Stern

Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match_driver

[Selvarasu Ganesan]

On 8/31/2024 9:59 AM, Alan Stern wrote:
> On Fri, Aug 30, 2024 at 06:16:12PM +0530, Selvarasu Ganesan wrote:
>> Hi Alan,
>>
>> Thanks for your comments. I understand your suggestions. We already have
>> a similar reference check with the udc name before calling
>> usb_gadget_register_driver.
>> In the drivers/usb/gadget/configfs.c file, I am wondering if there might
>> be an issue with the check of udc_name before
>> usb_gadget_register_driver. This is the only way to allow
>> gadget_register to be called before releasing or unregistering an
>> existing udc. Do you think we need to add an additional check here,
>> referencing the UDC, to prevent gadget_register from being called before
>> the existing UDC is released?
> I don't understand what you're saying.  There is no routine named
> "gadget_register".  (And there is no variable named "udc_name" in the
> code below, although there is gi->composite.gadget_driver.udc_name --
> but that's not a variable, it is a field in a structure.)
>
>> drivers/usb/gadget/configfs.c : gadget_dev_desc_UDC_store()
>> ===========================================================
>> if (gi->composite.gadget_driver.udc_name) {
>>                           ret = -EBUSY;
>>                           goto err;
>>                   }
>> gi->composite.gadget_driver.udc_name = name;
> Are you talking about this check and assignment?  Why do you think there
> might be a problem here?
>
> Are you worried that some UDC might be released while this code is
> running?  If that happens, why would it be a problem?


I am talking here based on the call traces, we are observing the 
following call traces at the time of failures. One specific point of 
interest is the gadget_match_driver() function, which is called as part 
of the usb_gadget_register_driver() function. I am wondering how the 
usb_gadget_register_driver() function allows the registration of a new 
driver even when an existing same UDC is not releasing. One possibility 
is that gi->composite.gadget_driver.udc_name becomes NULL before the UDC 
is released. However, as of now, we do not have any evidence to support 
this theory. We are still trying to reproduce the same issue with added 
more debugging logs.

CPU0: (ROLE SWITCH DEVICE <-> HOST)
==================================

->usb_role_switch_set_role()
  ->dwc3_usb_role_switch_set()
   ->dwc3_set_mode()
    ->__dwc3_set_mode()
     ->dwc3_gadget_exit()
      ->usb_del_gadget()
       ->device_unregister()
        ->put_device(dev)
         ->usb_udc_release()


  CPU1 (echo "<dwc3 device name>" > <path of udc 
config>/config/usb_gadget/g1/UDC)
=================================================================================
->configfs_write_iter()
  ->gadget_dev_desc_UDC_store()
   ->usb_gadget_register_driver()
    ->driver_register()
     ->bus_add_driver()
      ->driver_attach()
       ->bus_for_each_dev()
        ->__driver_attach()
         ->gadget_match_driver()

>
>> ret = usb_gadget_register_driver(&gi->composite.gadget_driver);
> Alan Stern
>