On Mon, Nov 25, 2024 at 10:53:46AM +0100, Arnd Bergmann wrote:
> On Mon, Nov 25, 2024, at 08:55, Andy Shevchenko wrote:
> > On Fri, Nov 22, 2024 at 08:24:37PM +0100, Arnd Bergmann wrote:
> >> On Fri, Nov 22, 2024, at 18:22, Guenter Roeck wrote:
> >>
> >> 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.
> >
> > Yep. the idea was to purely split this code out of the core
> > library parts. I was not intending any functional changes.
>
> Ok.
>
> > I believe it's a preexisted bug, but if you can point out to
> > the breakage I made, please do it, so I would be able to fix
> > ASAP.
> >
> >> 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.
> >
> > Btw, we have a new constant to tell the 8250 core that IO type is not set,
> > but that one is not used here.
>
> So I see serial8250_setup_port() setting "up->cur_iotype = 0xFF"
> by first calling serial8250_init_port(), this is the "not
> set" value you mean, right?.
Yes, and we have a constant for that, I'll send a patch to make it clear.
> Right after that it calls serial8250_set_defaults(),
> which sets "up->cur_iotype = p->iotype;", which may or
> may not be initialized here.
>
> The possible calls chains I see leading up to
> serial8250_setup_port() are:
>
> serial8250_register_8250_port(): here we first initialize
> the iotype incorrectly, then set uart->port.iotype and
> call serial8250_set_defaults() again to fix it.
>
> module_init(serial8250_init): relies on the first
> serial8250_set_defaults() for the ISA ports since they
> are always UPIO_PORT, but sets the other ones (pnp, acpi,
> platform_data) correctly.
>
> early_serial_setup(): called only on non-ISA platforms,
> shouldn't need to call serial8250_isa_init_ports() at
> all.
>
> console_initcall(univ8250_console_init): Not completely
> sure here, it seems this now only allows ports that
> are registered from one of the methods above
>
> Can you have a look at the patch below? I think this
> correctly removes the broken serial8250_set_defaults()
> while also still adding it in the one case that relies
> on the implied version.
>
> 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.
> 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..d3c1668add58 100644
> --- a/drivers/tty/serial/8250/8250_platform.c
> +++ b/drivers/tty/serial/8250/8250_platform.c
> @@ -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(port);
> +
> + /* Allow Intel CE4100 and jailhouse to override defaults */
> if (serial8250_isa_config != NULL)
> serial8250_isa_config(i, &up->port, &up->capabilities);
> }
--
With Best Regards,
Andy Shevchenko
