linux-kernel - Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in 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 for Android: free password hash cracker in your pocket

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

Message-ID: <2024092617-confined-liqueur-f4fa@gregkh>
Date: Thu, 26 Sep 2024 10:20:06 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: hbuczynski <hubert.buczynski94@...il.com>
Cc: balbi@...nel.org, quic_prashk@...cinc.com, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	"hubert.buczynski" <Hubert.Buczynski.ext@...g.de>
Subject: Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

On Thu, Sep 26, 2024 at 08:49:10AM +0200, hbuczynski wrote:
> From: "hubert.buczynski" <Hubert.Buczynski.ext@...g.de>

Please put your name here, not an email alias, same for the
signed-off-by line.

> The commit "5a444bea usb: gadget: u_serial: Set start_delayed during
> suspend" caused invocation of the gs_start_io in the gserial_resume.
> The gs_start_io doesn't check the ptr of the 'port.tty'. As a result, the
> tty_wakeup function is passed on to the NULL ptr causing kernel panic.
> 
> There is a deterministic scenario leading to the kernel error:
> 1. Set the device in peripheral OTG mode.
> 2. Attach the USB cable to the host.
> 3. Do not take any action on the host side.
> 4. Send data to the host, for example:
> $ echo "hello\n" > /dev/ttyGS0
> 5. Disconnect the USB cable.
> 6. Connect the USB cable and the kernel panic should be visible.
> 
> Fragment of the kernel panic log message:
> 
> Internal error: Oops: 5 [#1] PREEMPT SMP ARM
> Modules linked in:
> CPU: 0 PID: 0 Comm: swapper/0 Tainted: P O 5.15.166 #88
> Hardware name: STM32 hDevice Tree Support)
> PC is at tty_wakeup+0x8/0x5c
> LR is at gs_start_io+0x90/0xdc
> pc : [<c0623f74>]    lr : [<c083eeac>]    psr: 60010193
> sp : c1001da0  ip : c32e6944  fp : 80000000
> r10: c32e6934  r9 : c32e6928  r8 : c32e68ec
> r7 : c32e68e0  r6 : c2be6c40  r5 : 00000000  r4 : 00000000
> r3 : 00000000  r2 : 00000000  r1 : 60010193  r0 : 00000000
> Flags: nZC»  IRQs off  FIQs on  Mode SVC_32  ISA ARM Segment none
> Control: 10c5387d  Table: c3ac406a  DAC: 00000051
> Register r0 information: NULL pointer
> Register r1 information: non-paged memory
> Register r2 information: NULL pointer
> Register r3 information: NULL pointer
> Register r4 information: NULL pointer
> Register r5 information: NULL pointer
> [<c0623f74>] (tty_wakeup) from [<c083eeac>] (gs_start_io+0x90/0xdc)
> [<c083eeac>] (gs_start_io) from [<c083f0c0>](gserial_resume+0x6c/0xd4)
> [<c083f0c0>] (gserial_resume) from [<c082a35c>] (composite_resume+0x70/0x10c)
> [<c082a35c>] (composite_resume) from [<c082d668>] (configfs_composite_resume+0x54/0x64)
> [<c082d668>] (configfs_composite_resume) from [<c07c26c4>] (dwc2_handle_wakeup_detected_intr+0x15c/0x2e8)
> [<c07c26c4>] (dwc2_handle_wakeup_detected_intr) from [<c07c2c74>] (dwc2_handle_common_intr+0x424/0x630)
> [<c07c2c74>] (dwc2_handle_common_intr) from [<c0190168>] (__handle_irq_event_percpu+0x50/0x250)
> [<c0190168>] (__handle_irq_event_percpu) from [<c0190440>] (handle_irq_event+0x58/0xc4)
> [<c0190440>] (handle_irq_event) from [<c0194f9c>] (handle_fasteoi_irq+0x9c/0x204)
> [<c0194f9c>] (handle_fasteoi_irq) from [<c018fb2c>] (handle_domain_irq+0x58/0x74)
> [<c018fb2c>] (handle_domain_irq) from [<c0101328>] (gic_handle_irq+0x7c/0x90)
> [<c0101328>] (gic_handle_irq) from [<c0100b7c>] (__irq_svc+0x5c/0x90)
> 
> If the device sends data and does not receive msg from the host then the
> field port->read_started contains a positive value. After disconnecting
> the cable, gserial_suspend() is invoked and the port->start_delayed is set
> to true. Connecting the cable again causes invocation of the
> gserial_resume().
> The callstack after connecting the cable looks like the following:
> gserial_resume()
>   --> gs_start_io()
>     --> tty_wakeup() - with NULL argument
> 
> Fixes: 5a444bea37e2 ("usb: gadget: u_serial: Set start_delayed during suspend")
> 
> Signed-off-by: hubert.buczynski <Hubert.Buczynski.ext@...g.de>
> ---
>  drivers/usb/gadget/function/u_serial.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/u_serial.c b/drivers/usb/gadget/function/u_serial.c
> index 5111fcc0cac3..384f219fe01d 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -564,7 +564,7 @@ static int gs_start_io(struct gs_port *port)
>  	port->n_read = 0;
>  	started = gs_start_rx(port);
>  
> -	if (started) {
> +	if (started && port->port.tty) {
>  		gs_start_tx(port);

What ensures that port.tty does not change right after you check it
here?  Hasn't this been discussed a lot already in the archives?

thanks,

greg k-h