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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <27685942-fb62-44c5-86ed-98beb547ffed@roeck-us.net>
Date: Fri, 22 Nov 2024 09:22:35 -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
Subject: Re: [PATCH 1/1] tty: serial: handle HAS_IOPORT dependencies

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:
>>> On Fri, Apr 05, 2024 at 05:29:24PM +0200, Niklas Schnelle wrote:
>>>> In a future patch HAS_IOPORT=n will disable inb()/outb() and friends at
>>>> compile time. We thus need to add HAS_IOPORT as dependency for those
>>>> drivers using them unconditionally. For 8250 based drivers some support
>>>> MMIO only use so fence only the parts requiring I/O ports.
>>>>
>>>> Co-developed-by: Arnd Bergmann <arnd@...nel.org>
>>>> Signed-off-by: Arnd Bergmann <arnd@...nel.org>
>>>> Signed-off-by: Niklas Schnelle <schnelle@...ux.ibm.com>
>>> ...
>>>> @@ -422,10 +443,12 @@ static void set_io_from_upio(struct uart_port *p)
>>>>   	up->dl_write = default_serial_dl_write;
>>>>   
>>>> +	default:
>>>> +		WARN(1, "Unsupported UART type %x\n", p->iotype);
>>>
>>> So, according to this patch, the serial uart on microblaze, nios2,
>>> openrisc, xtensa, and possibly others is not or no longer supported.
>>>
>>> WARNING: CPU: 0 PID: 0 at drivers/tty/serial/8250/8250_port.c:470 serial8250_set_defaults+0x1a8/0x22c
>>> Unsupported UART type 0
>>>
>>> Any special reason ?
>>
>> So according to the warning the p->iotype is 0 which is UPIO_PORT.
>> For UPIO_PORT the switch above the WARN would pick io_serial_in() and
>> io_serial_out() as handlers. These use inb() respectively outb() to
>> access the serial so I don't see how they would work with !HAS_IOPORT
>> and it most definitely won't work for s390.
>>
>> Now for Microblaze Kconfig says to select HAS_IOPORT if PCI so I'd
>> assume that it can use inb()/outb() and maybe the PCI requirement is
>> not correct if this isn't a PCI device and it used to work with
>> inb()/outb()? For nios2, openrisc, and xtensa they don't select
>> HAS_IOPORT so either it really won't work anyway or they should select
>> it. Can you tell us more about the devices involved where you saw this?
> 
> I've tried to have a quick look at the four architectures, here
> is what I see in the sources:
> 
> - on microblaze, the default uart is xlnx,xps-uartlite-1.00.a,
>    and there is no 8250. The PCI code that theoretically supports
>    I/O port access through an ISA bridge (copied from powerpc),
>    but there is currently no code to set up the I/O space window,
>    so in practice the port I/O is just a NULL pointer dereference.
> 
> - nios2 has a 16550 compatible UART listed in the devicetree
>    file, but this uses normal UPIO_MEM registers, not ISA-style
>    I/O ports. There is no support for ISA or PCI.
> 
> - openrisc has no support for port I/O, like on nios2 the
>    16550 style uart is on UPIO_MEM according to the devicetree
> 
> - xtensa manually sets up a UPIO_MEM uart in its board files
>    and in its dts files. The PCI support does have code to
>    hand port I/O, but it looks incorrect to me and either broke
>    at some point or never worked. Most likely this was copied
>    incorrectly from old powerpc or sparc code where it worked.
> 
> 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 ?

Guenter