usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

PATCH In review hbuczynski 3 participants 6 messages Sep 26, 2024 USB

lore

Share & tools

lore

hbuczynski

author

2024-09-26 06:49 UTC #0 lore raw

PATCH 1/1 usb: gadget: u_serial: fix null-ptr-deref in gs_start_io

From: "hubert.buczynski" <[email protected]>

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 <[email protected]>

---
 drivers/usb/gadget/function/u_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/usb/gadget/function/u_serial.c +1 -1

index 5111fcc0cac3..384f219fe01d 100644
@@ -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() */

Signature · 1 line

2.25.1

Greg KH

maintainer

2024-09-26 08:20 UTC #1 lore raw

Quoted 1 line · hbuczynski

From: "hubert.buczynski" <[email protected]>

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

Quoted 70 lines

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 <[email protected]>
---
 drivers/usb/gadget/function/u_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/u_serial.c +0 -0
index 5111fcc0cac3..384f219fe01d 100644

drivers/usb/gadget/function/u_serial.c static int gs_start_io(struct gs_port *port) · line 568

Quoted 6 lines

drivers/usb/gadget/function/u_serial.c +1 -1

@@ -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

Prashanth K

2024-09-26 09:16 UTC #2 lore raw

In reply to hbuczynski

Quoted 72 lines · hbuczynski

From: "hubert.buczynski" <[email protected]>
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 <[email protected]>
---
 drivers/usb/gadget/function/u_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/u_serial.c +0 -0
index 5111fcc0cac3..384f219fe01d 100644

drivers/usb/gadget/function/u_serial.c static int gs_start_io(struct gs_port *port) · line 570

Quoted 8 lines

drivers/usb/gadget/function/u_serial.c +1 -1

@@ -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

Hubert Buczyński

author

2024-09-26 11:18 UTC #3 lore raw

Quoted 5 lines

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

Greg KH

maintainer

2024-09-26 12:25 UTC #4 lore raw

Quoted 6 lines · Hubert Buczyński

Quoted 5 lines
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

Quoted 6 lines

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

Prashanth K

2024-09-26 09:17 UTC #5 lore raw

In reply to hbuczynski

Quoted 7 lines · hbuczynski

From: "hubert.buczynski" <[email protected]>
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.

[...]

Quoted 23 lines

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 <[email protected]>
---
 drivers/usb/gadget/function/u_serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/function/u_serial.c +0 -0
index 5111fcc0cac3..384f219fe01d 100644

drivers/usb/gadget/function/u_serial.c static int gs_start_io(struct gs_port *port) · line 570

Quoted 8 lines

drivers/usb/gadget/function/u_serial.c +1 -1

@@ -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