Re: [PATCH 3/5] serial: mcf: dynamically allocate memory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tty: mcf: Use-After-Free and double-unregister due to dangling pointers and global array loop
- [High] tty: mcf: Premature console publication causes NULL pointer dereference
- [High] tty: serial: mcf: Use-After-Free on probe error and driver removal
--

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

serial: mcf: dynamically allocate memory

Modify the ColdFire serial driver to dynamically allocate the local uart
port structures as required at init time. Currently it uses a fixed
size array - a little wasteful of memory on most parts with only a
couple of UARTs.

> diff --git a/drivers/tty/serial/mcf.c b/drivers/tty/serial/mcf.c
> index 3651910dff99e..7cbe43cbe3a0e 100644
> --- a/drivers/tty/serial/mcf.c
> +++ b/drivers/tty/serial/mcf.c

[ ... ]

> @@ -509,9 +517,9 @@ static int __init mcf_console_setup(struct console *co, char *options)
>  
>  	if ((co->index < 0) || (co->index >= MCF_MAXPORTS))
>  		co->index = 0;
> -	port = &mcf_ports[co->index].port;
> -	if (port->membase == 0)
> +	if (mcf_ports[co->index] == NULL)
>  		return -ENODEV;
> +	port = &mcf_ports[co->index]->port;

[Severity: High]
Could this change lead to a NULL pointer dereference if console setup runs
concurrently with device probe?

In mcf_probe(), the new port structure is allocated and assigned to the
global mcf_ports[] array before port->membase is initialized via ioremap.
Since mcf_console_setup() no longer checks if port->membase is zero, it can
successfully complete before the mapping is done. If a printk occurs in
this window, mcf_console_putc() will dereference the NULL membase when
attempting to read the UART status.

[ ... ]

> @@ -572,10 +580,16 @@ static int mcf_probe(struct platform_device *pdev)
>  {
>  	struct uart_port *port;
>  	struct resource *res;
> +	struct mcf_uart *mp;
>  
>  	if (pdev->id >= MCF_MAXPORTS)
>  		return -ENODEV;
> -	port = &mcf_ports[pdev->id].port;
> +
> +	mp = devm_kzalloc(&pdev->dev, sizeof(*mp), GFP_KERNEL);
> +	if (mp == NULL)
> +		return -ENOMEM;
> +	mcf_ports[pdev->id] = mp;
> +	port = &mp->port;
>  
>  	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(port->membase))

[Severity: High]
Does this leak a dangling pointer into the global mcf_ports array if probe
fails?

The devm_kzalloc() pointer is assigned to mcf_ports[pdev->id] before
devm_platform_get_and_ioremap_resource() or platform_get_irq() are called.
If either of those functions fail, mcf_probe() returns an error, and the
devres framework will free the memory. However, mcf_ports[pdev->id] is
never cleared, leaving a dangling pointer that can be dereferenced later.

[ ... ]

> @@ -605,13 +619,11 @@ static int mcf_probe(struct platform_device *pdev)
>  
>  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);
> +		if (mcf_ports[i])
> +			uart_remove_one_port(&mcf_driver, &mcf_ports[i]->port);
>  	}
>  }

[Severity: High]
Can this loop cause a use-after-free and improperly unregister all ports
when a single device is removed?

During mcf_remove(), the code iterates through the entire mcf_ports array
up to MCF_MAXPORTS and calls uart_remove_one_port() on every non-NULL
entry, rather than just the port associated with the pdev being removed.

Additionally, it does not clear mcf_ports[i] back to NULL. When
mcf_remove() finishes, devres will free the memory associated with the
removed pdev. If another UART device is subsequently unbound, mcf_remove()
will run again, iterate over the array, and call uart_remove_one_port()
using the dangling pointer from the previously freed device.

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