linux-kernel - Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in gadget_match

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024082801-dissuade-starlight-e5ad@gregkh>
Date: Wed, 28 Aug 2024 11:39:58 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Selvarasu Ganesan <selvarasu.g@...sung.com>
Cc: stern@...land.harvard.edu, royluo@...gle.com, paul@...pouillou.net,
	elder@...nel.org, yuanlinyu@...onor.com, quic_kriskura@...cinc.com,
	crwulff@...il.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org, jh0801.jung@...sung.com,
	dh10.jung@...sung.com, naushad@...sung.com, akash.m5@...sung.com,
	rc93.raju@...sung.com, taehyun.cho@...sung.com,
	hongpooh.kim@...sung.com, eomji.oh@...sung.com,
	shijie.cai@...sung.com, stable <stable@...nel.org>
Subject: Re: [PATCH] usb: gadget: udc: Add null pointer check for udc in
 gadget_match_driver

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@...nel.org>
> Signed-off-by: Selvarasu Ganesan <selvarasu.g@...sung.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