Re: [PATCHv2 1/4] serial: mxs-auart: fix cast type for of_device_get_match_data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Out-of-bounds array write to negative index (-1) due to using pdev->id.
- [High] Use-after-unmap race condition due to mixing devm_request_irq with manual iounmap.
--

commit 65b6a79a32694ff06e9c834fc282d2c8989565ff
Author: Rosen Penev <rosenp@gmail.com>

serial: mxs-auart: fix cast type for of_device_get_match_data

of_device_get_match_data returns const void*. Cast to unsigned long to
avoid implicit integer truncation warnings. All the data parameters are
correct anyway.

> diff --git a/drivers/tty/serial/mxs-auart.c b/drivers/tty/serial/mxs-auart.c
> index 693b491f1e755..3e14c2768c48e 100644
> --- a/drivers/tty/serial/mxs-auart.c
> +++ b/drivers/tty/serial/mxs-auart.c
> @@ -1598,7 +1598,7 @@ static int mxs_auart_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  	}
>  
> -	s->devtype = (enum mxs_auart_type)of_device_get_match_data(&pdev->dev);
> +	s->devtype = (unsigned long)of_device_get_match_data(&pdev->dev);
>  
>  	ret = mxs_get_clks(s, pdev);
>  	if (ret)

[ ... ]

>  out_free_qpio_irq:
>  	mxs_auart_free_gpio_irq(s);
>  	auart_port[pdev->id] = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does this result in an
out-of-bounds write to a negative index?

For devices instantiated via Device Tree, pdev->id is initialized to
PLATFORM_DEVID_NONE (-1).

The driver correctly registers the port using auart_port[s->port.line] = s;
earlier in probe, but on the error path it attempts to clean up using
auart_port[pdev->id] = NULL;.

Could this corrupt the adjacent global memory in the BSS/Data section? It also
seems to fail to clear the actual dangling pointer at
auart_port[s->port.line], which might lead to a use-after-free later.

>  
>  out_iounmap:
>  	iounmap(s->port.membase);

[Severity: High]
This is a pre-existing issue, but does this manual iounmap() create a
use-after-unmap race condition?

The driver requests the main UART interrupt using devm_request_irq().
Devm-managed resources are freed automatically, but only after the probe
function returns.

Since the memory is manually unmapped here, there is a window where the
interrupt handler mxs_auart_irq_handle() remains registered and active.

If a spurious interrupt fires during this window, the handler could execute an
MMIO read via mxs_read(s, REG_STAT) on the unmapped memory, potentially causing
a kernel page fault.

[ ... ]

>  static void mxs_auart_remove(struct platform_device *pdev)
>  {
>  	struct mxs_auart_port *s = platform_get_drvdata(pdev);
>  
>  	uart_remove_one_port(&auart_driver, &s->port);
>  	auart_port[pdev->id] = NULL;

[Severity: High]
This is a pre-existing issue, but doesn't this also result in the same
out-of-bounds write to a negative index as seen in the probe error path?

>  	mxs_auart_free_gpio_irq(s);
>  	iounmap(s->port.membase);

[Severity: High]
This isn't a bug introduced by this patch, but does this create the same
use-after-unmap race condition during device removal?

Since the devm subsystem cleans up the IRQ handler after remove() completes,
manually unmapping the memory here leaves a window where an active interrupt
could trigger an MMIO read on the unmapped memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609223717.41670-2-rosenp@gmail.com?part=1