[PATCH] serial: 8250: Prevent kernel crash with nr_uarts=0

[PATCH] serial: 8250: Prevent kernel crash with nr_uarts=0

[Jan Kiszka]

When nr_uarts was set to 0 (via config or 8250_core.nr_uarts), we crash
early on x86 because serial8250_isa_init_ports dereferences base_ops
which remains NULL. In fact, there is nothing to do for that function if
there are no uarts.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/tty/serial/8250/8250_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 4506e40..e1363a40 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -3256,7 +3256,7 @@ static void __init serial8250_isa_init_ports(void)
 	static int first = 1;
 	int i, irqflag = 0;
 
-	if (!first)
+	if (!first || nr_uarts == 0)
 		return;
 	first = 0;
 
-- 
2.1.4

Re: [PATCH] serial: 8250: Prevent kernel crash with nr_uarts=0

[Peter Hurley]

Hi Jan,

On 05/04/2015 12:01 PM, Jan Kiszka wrote:
> When nr_uarts was set to 0 (via config or 8250_core.nr_uarts), we crash
> early on x86 because serial8250_isa_init_ports dereferences base_ops
> which remains NULL. In fact, there is nothing to do for that function if
> there are no uarts.

Thanks for finding this.

So nr_uarts == 0 effectively disables the 8250 driver. Is there any 
reason not to simply abort the driver init instead?

Regards,
Peter Hurley

> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/tty/serial/8250/8250_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 4506e40..e1363a40 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -3256,7 +3256,7 @@ static void __init serial8250_isa_init_ports(void)
>  	static int first = 1;
>  	int i, irqflag = 0;
>  
> -	if (!first)
> +	if (!first || nr_uarts == 0)
>  		return;
>  	first = 0;
>  
> 

Re: [PATCH] serial: 8250: Prevent kernel crash with nr_uarts=0

[Jan Kiszka]

On 2015-05-04 19:02, Peter Hurley wrote:
> Hi Jan,
> 
> On 05/04/2015 12:01 PM, Jan Kiszka wrote:
>> When nr_uarts was set to 0 (via config or 8250_core.nr_uarts), we crash
>> early on x86 because serial8250_isa_init_ports dereferences base_ops
>> which remains NULL. In fact, there is nothing to do for that function if
>> there are no uarts.
> 
> Thanks for finding this.
> 
> So nr_uarts == 0 effectively disables the 8250 driver. Is there any 
> reason not to simply abort the driver init instead?

I'm not very deep into this code, just stumbled over this while trying
some, well, unusual configurations.

If you prefer to handle this differently, I can recode it, of course.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH] serial: 8250: Prevent kernel crash with nr_uarts=0

[Peter Hurley]

On 05/04/2015 01:45 PM, Jan Kiszka wrote:
> On 2015-05-04 19:02, Peter Hurley wrote:
>> Hi Jan,
>>
>> On 05/04/2015 12:01 PM, Jan Kiszka wrote:
>>> When nr_uarts was set to 0 (via config or 8250_core.nr_uarts), we crash
>>> early on x86 because serial8250_isa_init_ports dereferences base_ops
>>> which remains NULL. In fact, there is nothing to do for that function if
>>> there are no uarts.
>>
>> Thanks for finding this.
>>
>> So nr_uarts == 0 effectively disables the 8250 driver. Is there any 
>> reason not to simply abort the driver init instead?
> 
> I'm not very deep into this code, just stumbled over this while trying
> some, well, unusual configurations.

Ok; I wasn't sure if this was related to some weird setup that needed
the 8250 driver loaded but in-op.

> If you prefer to handle this differently, I can recode it, of course.

Yes, please. We should bail out of any initialization if nr_uarts == 0.

That would be:
1. univ8250_console_init()
	The return value is ignored but the console should not be registered.

2. early_serial_setup()

	if (nr_uarts == 0)
		return -ENODEV;

3. serial8250_init()

	if (nr_uarts == 0)
		return -ENODEV;


Regards,
Peter Hurley

[PATCH] serial: 8250: Do nothing if nr_uarts=0

[Jan Kiszka]

When nr_uarts was set to 0 (via config or 8250_core.nr_uarts), we crash
early on x86 because serial8250_isa_init_ports dereferences base_ops
which remains NULL. In fact, there is nothing to do for all the callers
of serial8250_isa_init_ports if there are no uarts.

Based on suggestions by Peter Hurley.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

This supersedes "serial: 8250: Prevent kernel crash with nr_uarts=0"

 drivers/tty/serial/8250/8250_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 4506e40..dfcb14f 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -3548,6 +3548,9 @@ static struct console univ8250_console = {
 
 static int __init univ8250_console_init(void)
 {
+	if (nr_uarts == 0)
+		return -ENODEV;
+
 	serial8250_isa_init_ports();
 	register_console(&univ8250_console);
 	return 0;
@@ -3578,7 +3581,7 @@ int __init early_serial_setup(struct uart_port *port)
 {
 	struct uart_port *p;
 
-	if (port->line >= ARRAY_SIZE(serial8250_ports))
+	if (port->line >= ARRAY_SIZE(serial8250_ports) || nr_uarts == 0)
 		return -ENODEV;
 
 	serial8250_isa_init_ports();
@@ -3945,6 +3948,9 @@ static int __init serial8250_init(void)
 {
 	int ret;
 
+	if (nr_uarts == 0)
+		return -ENODEV;
+
 	serial8250_isa_init_ports();
 
 	printk(KERN_INFO "Serial: 8250/16550 driver, "
-- 
2.1.4