[PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[hbuczynski]

From: "hubert.buczynski" <Hubert.Buczynski.ext@feig.de>

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@feig.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);
 		/* Unblock any pending writes into our circular buffer, in case
 		 * we didn't in gs_start_tx() */
-- 
2.25.1

Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[Greg KH]

On Thu, Sep 26, 2024 at 08:49:10AM +0200, hbuczynski wrote:
> From: "hubert.buczynski" <Hubert.Buczynski.ext@feig.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@feig.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

Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[Prashanth K]

On 26-09-24 12:19 pm, hbuczynski wrote:
> From: "hubert.buczynski" <Hubert.Buczynski.ext@feig.de>
> 
> 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@feig.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);
>  		/* Unblock any pending writes into our circular buffer, in case
>  		 * we didn't in gs_start_tx() */

Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
gs_start_io") fixed this issue. Please try adding it into your builds.

Regards,
Prashanth K

Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[Hubert Buczyński]

> Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
> gs_start_io") fixed this issue. Please try adding it into your builds.
>
> Regards,
> Prashanth K

Thanks for the quick response. Indeed, the commit ffd603f21423
("usb: gadget: u_serial: Add null pointer check in gs_start_io")
solves the problem. Sorry for not checking the newest version.

I wonder whether it should also be applied to the LTS v5.15.167.
The bug is still present there.

Best Regards
Hubert

Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[Greg KH]

On Thu, Sep 26, 2024 at 01:17:53PM +0200, Hubert Buczyński wrote:
> > Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
> > gs_start_io") fixed this issue. Please try adding it into your builds.
> >
> > Regards,
> > Prashanth K
> 
> Thanks for the quick response. Indeed, the commit ffd603f21423
> ("usb: gadget: u_serial: Add null pointer check in gs_start_io")
> solves the problem. Sorry for not checking the newest version.
> 
> I wonder whether it should also be applied to the LTS v5.15.167.
> The bug is still present there.

Then submit it for inclusion as per the stable kernel rules!

thanks,

greg k-h

Re: [PATCH] usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

[Prashanth K]

On 26-09-24 12:19 pm, hbuczynski wrote:
> From: "hubert.buczynski" <Hubert.Buczynski.ext@feig.de>
> 
> 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.
> 
[...]
> 
> 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@feig.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);
>  		/* Unblock any pending writes into our circular buffer, in case
>  		 * we didn't in gs_start_tx() */

Commit ffd603f21423 ("usb: gadget: u_serial: Add null pointer check in
gs_start_io") fixed this issue. Please try adding it into your builds.

Regards,
Prashanth K