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;
+
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
