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: <744920ba-551c-466b-ac5c-2607b81261a0@app.fastmail.com>
Date: Fri, 22 Nov 2024 20:24:37 +0100
From: "Arnd Bergmann" <arnd@...nel.org>
To: "Guenter Roeck" <linux@...ck-us.net>,
 "Niklas Schnelle" <schnelle@...ux.ibm.com>
Cc: "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,
 "Andy Shevchenko" <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
> On 11/22/24 08:31, Arnd Bergmann wrote:
>> On Fri, Nov 22, 2024, at 16:35, Niklas Schnelle wrote:
>>> On Fri, 2024-11-22 at 07:18 -0800, Guenter Roeck wrote:
>> So in all four cases, the normal uart should keep working,
>> and if something tried to start up an ISA style 8250, I
>> would expect to see the new version produce the WARN()
>> in place of what was a NULL pointer dereference (reading
>> from (u8 *)0x2f8) before.
>> 
>> Since there are so many ways to set up a broken 8250,
>> it is still possible that something tries to add another
>> UPIO_PORT uart, and that this causes the WARN() to trigger,
>> if we find any of those, the fix is to no stop registering
>> broken ports.
>> 
>
> The call chain in all cases is
>
> [    0.013596] Call Trace:
> [    0.013796]  [<d06eb249>] dump_stack+0x9/0xc
> [    0.014115]  [<d000c12c>] __warn+0x94/0xbc
> [    0.014332]  [<d000c29c>] warn_slowpath_fmt+0x148/0x184
> [    0.014560]  [<d04f03d8>] set_io_from_upio+0x70/0x98
> [    0.014781]  [<d04f0459>] serial8250_set_defaults+0x59/0x8c
> [    0.015013]  [<d04efa6a>] serial8250_setup_port+0x6e/0x90
> [    0.015240]  [<d0ae2a12>] serial8250_isa_init_ports+0x32/0x5c
> [    0.015473]  [<d0ae28a1>] univ8250_console_init+0x15/0x24
> [    0.015698]  [<d0ad0684>] console_init+0x18/0x20
> [    0.015926]  [<d0acbf43>] start_kernel+0x3db/0x4cc
> [    0.016145]  [<d06ebc37>] _startup+0x13b/0x13b
>
> That seems unconditional. What is the architecture expected to do to
> prevent this call chain from being executed ?

This looks like a bug in drivers/tty/serial/8250/8250_platform.c
to me, not something the architectures did wrong. My impression
from __serial8250_isa_init_ports() is that this is supposed
to initialize exactly the ports in the old_serial_port[]
array, which is filled only on alpha, m68k and x86 but not
on the other ones.

Andy moved this code in ffd8e8bd26e9 ("serial: 8250: Extract
platform driver"), but I don't think this move by itself
changed anything.

serial8250_setup_port() is where it ends up using the
uninitialized serial8250_ports[index] contents. Since 
port->iotype is not set to anything here, it defaults to
'0', which happens to be UPIO_PORT.

The reason it doesn't immediately crash and burn is that
this is still only setting up the structures for later
use, but I assume that trying to use console=ttyS0, or
opening /dev/ttyS0 on the uninitialized port would
then cause an oops.

The bit I'm less sure about is why the
serial8250_setup_port() function is called here in
the first place. I assume it does something for
/some/ architecture, but it's clearly wrong for
most of them.

       Arnd