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: <791f37f3-2b93-47f1-b0a3-46825c09f222@roeck-us.net>
Date: Fri, 22 Nov 2024 12:44:33 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Arnd Bergmann <arnd@...nel.org>, 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 11/22/24 11:24, Arnd Bergmann wrote:
> 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.
> 

All four affected platforms use ttyS0, only it is mmio based,
not io port based.

Guenter

> 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