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

[Selvarasu Ganesan <selvarasu.g@samsung.com>]

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
>