| Message ID | 20210426161433.20829-1-vegard.nossum@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | serial: 8250: fix NULL pointer dereference in serial8250_do_startup() | expand |
On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote: > Syzbot reported a crash, here reproduced on a recent mainline kernel: > > BUG: kernel NULL pointer dereference, address: 0000000000000005 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 120cf067 P4D 120cf067 PUD 135d4067 PMD 0 > Oops: 0000 [#1] PREEMPT SMP KASAN > CPU: 2 PID: 4830 Comm: a.out Not tainted 5.12.0-rc7+ #209 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 > RIP: 0010:mem16_serial_in+0x83/0xa0 > [...] > Call Trace: > serial8250_do_startup+0x475/0x1e40 > serial8250_startup+0x5c/0x80 > uart_startup+0x360/0x870 > uart_set_info_user+0x13a3/0x1c30 > tty_ioctl+0x711/0x14f0 > __x64_sys_ioctl+0x193/0x200 > do_syscall_64+0x2d/0x70 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > A more readable reproducer is: > > #include <sys/ioctl.h> > #include <fcntl.h> > > #include <linux/serial.h> > > #ifndef SERIAL_IO_MEM16 > #define SERIAL_IO_MEM16 7 > #endif > > int main(int argc, char *argv[]) > { > int fd = open("/dev/ttyS3", O_RDONLY); > > struct serial_struct ss = {}; > ss.type = 0x10; > ss.baud_base = 0x7fffffff; > ss.io_type = SERIAL_IO_MEM16; > ioctl(fd, TIOCSSERIAL, &ss); > > return 0; > } > > ioctl(TIOCSSERIAL) attempts to configure the serial port, but when > requesting io_type SERIAL_IO_MEM*/UPIO_MEM* it goes on to dereference > ->membase in serial8250_do_startup(). > > I propose this fix, which will fail validation of the TIOCSSERIAL request > if you request a memory-based or io-based io_type when the underlying port > has no valid ->membase or ->iobase, respectively. > > As far as I can tell, this driver was written to support being able to > switch between the two IO types for a given port (assuming the underlying > driver supports it); see serial8250_do_startup()/set_io_from_upio(). > > I'm also adding a couple of WARN_ON_ONCE()s which are technically > redundant, but which could help somebody else if they come across a > similar issue in the future. > > Reported-by: syzbot+4c7f1a69dfe24c6b3aeb@syzkaller.appspotmail.com > Cc: Peter Hurley <peter@hurleysoftware.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: linux-serial@vger.kernel.org > Cc: Caleb Connolly <caleb@connolly.tech> > Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com> > --- > drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++++++++++++++++++++++++ > 1 file changed, 43 insertions(+) > > diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c > index b0af13074cd36..aec3abff8e48e 100644 > --- a/drivers/tty/serial/8250/8250_port.c > +++ b/drivers/tty/serial/8250/8250_port.c > @@ -455,6 +455,33 @@ static void io_serial_out(struct uart_port *p, int offset, int value) > > static int serial8250_default_handle_irq(struct uart_port *port); > > +static int needs_membase(int iotype) > +{ > + switch (iotype) { > + case UPIO_MEM: > + case UPIO_MEM16: > + case UPIO_MEM32: > + case UPIO_MEM32BE: > +#ifdef CONFIG_SERIAL_8250_RT288X > + case UPIO_AU: > +#endif > + return 1; > + } > + > + return 0; > +} > + > +static int needs_iobase(int iotype) > +{ > + switch (iotype) { > + case UPIO_HUB6: > + case UPIO_PORT: > + return 1; > + } > + > + return 0; > +} > + > static void set_io_from_upio(struct uart_port *p) > { > struct uart_8250_port *up = up_to_u8250p(p); > @@ -2151,6 +2178,11 @@ int serial8250_do_startup(struct uart_port *port) > unsigned char lsr, iir; > int retval; > > + if (WARN_ON_ONCE(needs_membase(port->iotype) && !port->membase)) > + return -ENODEV; > + if (WARN_ON_ONCE(needs_iobase(port->iotype) && !port->iobase)) > + return -ENODEV; These WARN_ON() will still trigger syzbot. Are you sure you tested this and had syzbot verify it? thanks, greg k-h
On Mon, Apr 26, 2021 at 06:33:01PM +0200, Vegard Nossum wrote: > On 2021-04-26 18:17, Greg Kroah-Hartman wrote: > > On Mon, Apr 26, 2021 at 06:14:33PM +0200, Vegard Nossum wrote: > > > static void set_io_from_upio(struct uart_port *p) > > > { > > > struct uart_8250_port *up = up_to_u8250p(p); > > > @@ -2151,6 +2178,11 @@ int serial8250_do_startup(struct uart_port *port) > > > unsigned char lsr, iir; > > > int retval; > > > + if (WARN_ON_ONCE(needs_membase(port->iotype) && !port->membase)) > > > + return -ENODEV; > > > + if (WARN_ON_ONCE(needs_iobase(port->iotype) && !port->iobase)) > > > + return -ENODEV; > > > > These WARN_ON() will still trigger syzbot. Are you sure you tested this > > and had syzbot verify it? > > I tested it locally and the WARN_ON()s don't trigger -- presumably > because serial8250_verify_port() is called from uart_set_info() before > we get to serial8250_do_startup(): > > /* > * Ask the low level driver to verify the settings. > */ > if (uport->ops->verify_port) > retval = uport->ops->verify_port(uport, new_info); > > [...] > > retval = uart_startup(tty, state, 1); > > At least, this was my intention. Although now that I look at it again, > it looks like this check may be skipped in some cases; is that what > you're referring to? > > I didn't have syzbot verify it -- I thought it would do that when > submitting my patch. Looks like I need to push somewhere and ask syzbot > to test it using this? > > #syz test: git://repo/address.git commit-hash > > (I assume I can send this privately as long as I use the right > syzbot+...@ To-address?) > Dropping this now until you get this tested properly... thanks, greg k-h
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index b0af13074cd36..aec3abff8e48e 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -455,6 +455,33 @@ static void io_serial_out(struct uart_port *p, int offset, int value) static int serial8250_default_handle_irq(struct uart_port *port); +static int needs_membase(int iotype) +{ + switch (iotype) { + case UPIO_MEM: + case UPIO_MEM16: + case UPIO_MEM32: + case UPIO_MEM32BE: +#ifdef CONFIG_SERIAL_8250_RT288X + case UPIO_AU: +#endif + return 1; + } + + return 0; +} + +static int needs_iobase(int iotype) +{ + switch (iotype) { + case UPIO_HUB6: + case UPIO_PORT: + return 1; + } + + return 0; +} + static void set_io_from_upio(struct uart_port *p) { struct uart_8250_port *up = up_to_u8250p(p); @@ -2151,6 +2178,11 @@ int serial8250_do_startup(struct uart_port *port) unsigned char lsr, iir; int retval; + if (WARN_ON_ONCE(needs_membase(port->iotype) && !port->membase)) + return -ENODEV; + if (WARN_ON_ONCE(needs_iobase(port->iotype) && !port->iobase)) + return -ENODEV; + if (!port->fifosize) port->fifosize = uart_config[port->type].fifo_size; if (!up->tx_loadsz) @@ -3157,6 +3189,17 @@ serial8250_verify_port(struct uart_port *port, struct serial_struct *ser) ser->type >= ARRAY_SIZE(uart_config) || ser->type == PORT_CIRRUS || ser->type == PORT_STARTECH) return -EINVAL; + + /* + * This driver clearly was intended to support switching between + * io types (see serial8250_do_startup()), so we need to ensure that + * the underlying port type will support the request. + */ + if (needs_membase(ser->io_type) && !port->membase) + return -EINVAL; + if (needs_iobase(ser->io_type) && !port->iobase) + return -EINVAL; + return 0; }
Syzbot reported a crash, here reproduced on a recent mainline kernel: BUG: kernel NULL pointer dereference, address: 0000000000000005 #PF: supervisor read access in kernel mode #PF: error_code(0x0000) - not-present page PGD 120cf067 P4D 120cf067 PUD 135d4067 PMD 0 Oops: 0000 [#1] PREEMPT SMP KASAN CPU: 2 PID: 4830 Comm: a.out Not tainted 5.12.0-rc7+ #209 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014 RIP: 0010:mem16_serial_in+0x83/0xa0 [...] Call Trace: serial8250_do_startup+0x475/0x1e40 serial8250_startup+0x5c/0x80 uart_startup+0x360/0x870 uart_set_info_user+0x13a3/0x1c30 tty_ioctl+0x711/0x14f0 __x64_sys_ioctl+0x193/0x200 do_syscall_64+0x2d/0x70 entry_SYSCALL_64_after_hwframe+0x44/0xae A more readable reproducer is: #include <sys/ioctl.h> #include <fcntl.h> #include <linux/serial.h> #ifndef SERIAL_IO_MEM16 #define SERIAL_IO_MEM16 7 #endif int main(int argc, char *argv[]) { int fd = open("/dev/ttyS3", O_RDONLY); struct serial_struct ss = {}; ss.type = 0x10; ss.baud_base = 0x7fffffff; ss.io_type = SERIAL_IO_MEM16; ioctl(fd, TIOCSSERIAL, &ss); return 0; } ioctl(TIOCSSERIAL) attempts to configure the serial port, but when requesting io_type SERIAL_IO_MEM*/UPIO_MEM* it goes on to dereference ->membase in serial8250_do_startup(). I propose this fix, which will fail validation of the TIOCSSERIAL request if you request a memory-based or io-based io_type when the underlying port has no valid ->membase or ->iobase, respectively. As far as I can tell, this driver was written to support being able to switch between the two IO types for a given port (assuming the underlying driver supports it); see serial8250_do_startup()/set_io_from_upio(). I'm also adding a couple of WARN_ON_ONCE()s which are technically redundant, but which could help somebody else if they come across a similar issue in the future. Reported-by: syzbot+4c7f1a69dfe24c6b3aeb@syzkaller.appspotmail.com Cc: Peter Hurley <peter@hurleysoftware.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-serial@vger.kernel.org Cc: Caleb Connolly <caleb@connolly.tech> Signed-off-by: Vegard Nossum <vegard.nossum@oracle.com> --- drivers/tty/serial/8250/8250_port.c | 43 +++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+)