| Message ID | 20240926064910.17429-1-hubert.buczynski94@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: u_serial: fix null-ptr-deref in gs_start_io | expand |
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
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
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
> 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
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
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() */