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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Cc: Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-serial@xxxxxxxxxxxxxxx
> Cc: Caleb Connolly <caleb@xxxxxxxxxxxxx>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxxx>
> ---
> 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
