linux-kernel - Re: [PATCH v2] usb: gadget: u_serial: Fix the issue that gs_start

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: <b0c787ad-55a1-4fb1-b9cd-1f688ea65316@quicinc.com>
Date: Fri, 22 Nov 2024 13:53:22 +0530
From: Prashanth K <quic_prashk@...cinc.com>
To: 胡连勤 <hulianqin@...o.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "quic_jjohnson@...cinc.com" <quic_jjohnson@...cinc.com>,
        "mwalle@...nel.org"
	<mwalle@...nel.org>
CC: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        opensource.kernel <opensource.kernel@...o.com>
Subject: Re: [PATCH v2] usb: gadget: u_serial: Fix the issue that gs_start_io
 crashed due to accessing null pointer



On 21-11-24 06:11 pm, 胡连勤 wrote:
> From: Lianqin Hu <hulianqin@...o.com>
> 
> Considering that in some extreme cases,
> when u_serial driver is accessed by multiple threads,
> Thread A is executing the open operation and calling the gs_open,
> Thread B is executing the disconnect operation and calling the
> gserial_disconnect function,The port->port_usb pointer will be set to NULL.
> 
> E.g.
>     Thread A                                 Thread B
> 
>     gs_open()                                gadget_unbind_driver()
> 
You can remove these extra lines.
>     gs_start_io()                            composite_disconnect()
> 
>     gs_start_rx()                            gserial_disconnect()
>     ...                                      ...
>     spin_unlock(&port->port_lock)
>     status = usb_ep_queue()                  spin_lock(&port->port_lock)
>     spin_lock(&port->port_lock)              port->port_usb = NULL
>     gs_free_requests(port->port_usb->in)     spin_unlock(&port->port_lock)
>     Crash
> 
> This causes thread A to access a null pointer (port->port_usb is null)
> when calling the gs_free_requests function, causing a crash.
> 
> To avoid this, add a null pointer check to gs_start_io before attempting
> to access the value of the pointer port->port_usb.
> 
> Unable to handle kernel NULL pointer dereference at
> virtual address 00000000000000e8
> pc : gs_start_io+0x164/0x25c
> lr : gs_start_io+0x238/0x25c
> sp : ffffffc08b75ba00
> x29: ffffffc08b75ba00 x28: ffffffed8ba01000 x27: 0000000000020902
> x26: dead000000000100 x25: ffffff899f43a400 x24: ffffff8862325400
> x23: ffffff88623256a4 x22: ffffff8862325690 x21: ffffff88623255ec
> x20: ffffff88623255d8 x19: ffffff885e19d700 x18: ffffffed8c45ae40
> x17: 00000000d48d30ad x16: 00000000d48d30ad x15: 0010000000000001
> x14: ffffffed8c50fcc0 x13: 0000000040000000 x12: 0000000000000001
> x11: 0000000080200012 x10: 0000000080200012 x9 : ffffff88623255d8
> x8 : 0000000000000000 x7 : 0000000000000000 x6 : 000000000000003f
> x5 : ffffffed8ae0b9a4 x4 : fffffffe267d0ea0 x3 : 0000000080200012
> x2 : ffffff899f43a400 x1 : 0000000080200013 x0 : ffffff899f43b100

These address lists can also omitted. And the below stack can be shortened.

> Call trace:
>  gs_start_io+0x164/0x25c
>  gs_open+0x108/0x13c
>  tty_open+0x314/0x638
>  chrdev_open+0x1b8/0x258
>  do_dentry_open+0x2c4/0x700
>  vfs_open+0x2c/0x3c
>  path_openat+0xa64/0xc60
>  do_filp_open+0xb8/0x164
>  do_sys_openat2+0x84/0xf0
>  __arm64_sys_openat+0x70/0x9c
>  invoke_syscall+0x58/0x114
>  el0_svc_common+0x80/0xe0
>  do_el0_svc+0x1c/0x28
>  el0_svc+0x38/0x68
>  el0t_64_sync_handler+0x68/0xbc
>  el0t_64_sync+0x1a8/0x1ac
> Code: f2fbd5ba eb14013f 540004a1 f940e708 (f9407513)
> ---[ end trace 0000000000000000 ]---
> 
> Suggested-by: Prashanth K <quic_prashk@...cinc.com>
> Signed-off-by: Lianqin Hu <hulianqin@...o.com>

Include fixes tag and CC stable kernel.
> 
> v2:
>  - Modify patch content and description according to "v1 suggestion"
>  - Link to v1: https://lore.kernel.org/all/TYUPR06MB621737D16F68B5ABD6CF5772D2272@TYUPR06MB6217.apcprd06.prod.outlook.com/
> 
>  drivers/usb/gadget/function/u_serial.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 0a8c05b2746b..53d9fc41acc5 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -579,9 +579,12 @@ static int gs_start_io(struct gs_port *port)
>  		 * we didn't in gs_start_tx() */
>  		tty_wakeup(port->port.tty);
>  	} else {
> -		gs_free_requests(ep, head, &port->read_allocated);
> -		gs_free_requests(port->port_usb->in, &port->write_pool,
> -			&port->write_allocated);
> +		/* Free reqs only if we are still connected */
> +		if (port->port_usb) {
> +			gs_free_requests(ep, head, &port->read_allocated);
> +			gs_free_requests(port->port_usb->in, &port->write_pool,
> +				&port->write_allocated);
You can also mention in commit text that skip freeing reqs if port_usb
is null, since it will be done by gser_disconnect.  Maybe you can wait
for other people to review before sending next version.
> +		}
>  		status = -EIO;
>  	}
>  
Regards,
Prashanth K