linux-kernel - Re: [PATCH 1/1] tty: serial: handle HAS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <be43108e-4135-4927-ba58-141836fde6af@app.fastmail.com>
Date: Mon, 25 Nov 2024 12:06:16 +0100
From: "Arnd Bergmann" <arnd@...nel.org>
To: "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>
Cc: "Guenter Roeck" <linux@...ck-us.net>,
 "Niklas Schnelle" <schnelle@...ux.ibm.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Jiri Slaby" <jirislaby@...nel.org>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 linux-serial@...r.kernel.org, "Heiko Carstens" <hca@...ux.ibm.com>,
 linux-kernel@...r.kernel.org, "Yang Yingliang" <yangyingliang@...wei.com>
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

On Mon, Nov 25, 2024, at 11:33, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
>> 
>> This does however revert f4c23a140d80 ("serial: 8250:
>> fix null-ptr-deref in serial8250_start_tx()") and
>> might bring back the bug came from opening an
>> uninitialized uart. On the other hand, I don't see
>> why that doesn't also crash from accessing the invalid
>> I/O port on the same architectures.
>
> AFAICS it does only partial revert, so I don't see how your patch
> may break that.

I think it's a complete revert, it's just not entirely obvious
since serial8250_setup_port() got split out by 9d86719f8769
("serial: 8250: Allow using ports higher than
SERIAL_8250_RUNTIME_UARTS") and the original callsite got
moved by your ffd8e8bd26e9 ("serial: 8250: Extract platform
driver").

What I suspect is going on with the f4c23a140d80 commit
is the same bug I mentioned earlier in this thread, where
__serial8250_isa_init_ports() just always registers
'nr_uarts' (CONFIG_SERIAL_8250_RUNTIME_UARTS) ports,
unlike any other serial driver.

This used to be required before 9d86719f8769 ("serial:
8250: Allow using ports higher than SERIAL_8250_RUNTIME_UARTS"),
but I don't see why this is still a thing now, other than
for using setserial on i486-class PCs with nonstandard ISA
ports.

On non-x86 machines, it only ever seems to create extra
ports that are likely to crash the system if opened, either
because they lack proper serial_in/serial_out callbacks,
or because the default UPIO_PORT callbacks end up poking
unmapped memory.

Do you see any reason why we can't just do the version below?

        Arnd

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 5f9f06911795..5b72309611cb 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -358,8 +358,6 @@ struct uart_8250_port *serial8250_setup_port(int index)
 
 	up->ops = &univ8250_driver_ops;
 
-	serial8250_set_defaults(up);
-
 	return up;
 }
 
diff --git a/drivers/tty/serial/8250/8250_platform.c b/drivers/tty/serial/8250/8250_platform.c
index 66fd6d5d4835..7675536b6396 100644
--- a/drivers/tty/serial/8250/8250_platform.c
+++ b/drivers/tty/serial/8250/8250_platform.c
@@ -33,8 +33,6 @@
 unsigned int share_irqs = SERIAL8250_SHARE_IRQS;
 unsigned int skip_txen_test;
 
-unsigned int nr_uarts = CONFIG_SERIAL_8250_RUNTIME_UARTS;
-
 #include <asm/serial.h>
 
 /*
@@ -50,6 +48,8 @@ static const struct old_serial_port old_serial_port[] = {
 	SERIAL_PORT_DFNS /* defined in asm/serial.h */
 };
 
+unsigned int nr_uarts = ARRAY_SIZE(old_serial_port);;
+
 serial8250_isa_config_fn serial8250_isa_config;
 void serial8250_set_isa_configurator(serial8250_isa_config_fn v)
 {
@@ -65,9 +65,9 @@ static void __init __serial8250_isa_init_ports(void)
 		nr_uarts = UART_NR;
 
 	/*
-	 * Set up initial ISA ports based on nr_uart module param, or else
-	 * default to CONFIG_SERIAL_8250_RUNTIME_UARTS. Note that we do not
-	 * need to increase nr_uarts when setting up the initial ISA ports.
+	 * Set up initial ISA ports based on nr_uart module param. Note that 
+	 * we do not need to increase nr_uarts when setting up the initial
+	 * ISA ports.
 	 */
 	for (i = 0; i < nr_uarts; i++)
 		serial8250_setup_port(i);
@@ -94,6 +94,10 @@ static void __init __serial8250_isa_init_ports(void)
 		port->regshift = old_serial_port[i].iomem_reg_shift;
 
 		port->irqflags |= irqflag;
+
+		serial8250_set_defaults(up);
+
+		/* Allow Intel CE4100 and jailhouse to override defaults */
 		if (serial8250_isa_config != NULL)
 			serial8250_isa_config(i, &up->port, &up->capabilities);
 	}
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index 5b1cc009b977..3bf27cecf82e 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -198,17 +198,6 @@ config SERIAL_8250_NR_UARTS
 	  PCI enumeration and any ports that may be added at run-time
 	  via hot-plug, or any ISA multi-port serial cards.
 
-config SERIAL_8250_RUNTIME_UARTS
-	int "Number of 8250/16550 serial ports to register at runtime"
-	depends on SERIAL_8250
-	range 0 SERIAL_8250_NR_UARTS
-	default "4"
-	help
-	  Set this to the maximum number of serial ports you want
-	  the kernel to register at boot time.  This can be overridden
-	  with the module parameter "nr_uarts", or boot-time parameter
-	  8250.nr_uarts
-
 config SERIAL_8250_EXTENDED
 	bool "Extended 8250/16550 serial driver options"
 	depends on SERIAL_8250