Re: [PATCH 2/5] serial: mcf: convert to use proper platform resources

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: mcf: unconditional removal of all UART ports in mcf_remove()
- [High] tty: mcf: ERR_PTR stored in membase bypasses uninitialized check
- [Medium] tty: mcf: tautological error check on platform_get_irq()
--

commit c5cb7916e270b19e0758d11e193eb23c71763eca
Author: Greg Ungerer <gerg@linux-m68k.org>

serial: mcf: convert to use proper platform resources

The ColdFire SoC device setup code supplies a non-standard array of
hardware address information (memory address and IRQ) for initializing
the UARTS - which it does as a single blob. The mcf serial probe
function processes the whole set in one go from a single platform entry.

[ ... ]
> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
> index 2b862dfe39ab3..3651910dff99e 100644
> --- a/drivers/tty/serial/mcf.c
> +++ b/drivers/tty/serial/mcf.c
[ ... ]
> @@ -570,31 +570,34 @@ static struct uart_driver mcf_driver = {
>  
>  static int mcf_probe(struct platform_device *pdev)
>  {
> -	struct mcf_platform_uart *platp = dev_get_platdata(&pdev->dev);
>  	struct uart_port *port;
> -	int i;
> +	struct resource *res;
>  
> -	for (i = 0; ((i < MCF_MAXPORTS) && (platp[i].mapbase)); i++) {
> -		port = &mcf_ports[i].port;
> +	if (pdev->id >= MCF_MAXPORTS)
> +		return -ENODEV;
> +	port = &mcf_ports[pdev->id].port;
>  
> -		port->line = i;
> -		port->type = PORT_MCF;
> -		port->mapbase = platp[i].mapbase;
> -		port->membase = (platp[i].membase) ? platp[i].membase :
> -			(unsigned char __iomem *) platp[i].mapbase;
> -		port->dev = &pdev->dev;
> -		port->iotype = SERIAL_IO_MEM;
> -		port->irq = platp[i].irq;
> -		port->uartclk = MCF_BUSCLK;
> -		port->ops = &mcf_uart_ops;
> -		port->flags = UPF_BOOT_AUTOCONF;
> -		port->rs485_config = mcf_config_rs485;
> -		port->rs485_supported = mcf_rs485_supported;
> -		port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
> -
> -		uart_add_one_port(&mcf_driver, port);
> -	}
> +	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(port->membase))
> +		return PTR_ERR(port->membase);

[Severity: High]
Since port is a pointer to the global mcf_ports array, assigning an ERR_PTR
to port->membase leaves the error pointer in the global state if
devm_platform_get_and_ioremap_resource() fails.

Later, when the early console is initialized, mcf_console_setup() checks if
membase is valid using:

    if (port->membase == 0)
        return -ENODEV;

Since an ERR_PTR is not zero, this check will pass, which could cause a
kernel panic when mcf_console_write() dereferences the ERR_PTR during a
printk.

Could we store the mapped address in a local variable and only assign it to
port->membase on success?

> +	port->mapbase = res->start;
> +
> +	port->irq = platform_get_irq(pdev, 0);
> +	if (port->irq < 0)
> +		return port->irq;

[Severity: Medium]
In struct uart_port, irq is defined as an unsigned int. Does the check
(port->irq < 0) undergo unsigned comparison and become tautologically false?

If platform_get_irq() fails and returns a negative error code like -ENXIO,
it seems this error would be swallowed, and the port would be registered
with a bogus, very large IRQ number.

> +	port->line = pdev->id;
> +	port->type = PORT_MCF;
> +	port->dev = &pdev->dev;
> +	port->iotype = SERIAL_IO_MEM;
> +	port->uartclk = MCF_BUSCLK;
> +	port->ops = &mcf_uart_ops;
> +	port->flags = UPF_BOOT_AUTOCONF;
> +	port->rs485_config = mcf_config_rs485;
> +	port->rs485_supported = mcf_rs485_supported;
> +	port->has_sysrq = IS_ENABLED(CONFIG_SERIAL_MCF_CONSOLE);
> +
> +	uart_add_one_port(&mcf_driver, port);
>  	return 0;
>  }

[Severity: High]
With the conversion of mcf_probe() to process a single platform device per
call instead of a single blob, does mcf_remove() also need to be updated?

Looking at mcf_remove() in drivers/tty/serial/mcf.c:

static void mcf_remove(struct platform_device *pdev)
{
	struct uart_port *port;
	int i;

	for (i = 0; (i < MCF_MAXPORTS); i++) {
		port = &mcf_ports[i].port;
		if (port)
			uart_remove_one_port(&mcf_driver, port);
	}
}

It appears this loop still unconditionally unregisters all 10 ports,
regardless of which pdev is being removed. If a single device is unbound
via sysfs, will this disrupt other devices or lead to list corruption by
calling uart_remove_one_port() on uninitialized structs or multiple times
on the same port?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013120831.1046590-1-gerg@linux-m68k.org?part=2