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 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
>