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@xxxxxxxxxx>
> > Signed-off-by: Selvarasu Ganesan <selvarasu.g@xxxxxxxxxxx>
> > ---
> > 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
