Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vegard Nossum <vegard.nossum@oracle.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial@vger.kernel.org,
        syzbot+4c7f1a69dfe24c6b3aeb@syzkaller.appspotmail.com,
        syzbot+92f32d4e21fb246d31a2@syzkaller.appspotmail.com
Cc:     linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com,
        Vegard Nossum <vegard.nossum@oracle.com>,
        Peter Hurley <peter@hurleysoftware.com>,
        Caleb Connolly <caleb@connolly.tech>
Subject: [PATCH] serial: 8250: fix NULL pointer dereference in serial8250_do_startup()
Date:   Mon, 26 Apr 2021 18:14:33 +0200
Message-Id: <20210426161433.20829-1-vegard.nossum@oracle.com>
In-Reply-To: <00000000000044a65205994a7e13@google.com>
References: <00000000000044a65205994a7e13@google.com>
Precedence: bulk

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;
+
 	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;
 }
 
-- 
2.16.1.72.g5be1f00a9.dirty