Re: [PATCH v3 3/7] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

[sashiko-bot@kernel.org]

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

New issues:
- [High] Direct return in the newly added AST2600 PCI initialization logic bypasses all resource cleanup.

Pre-existing issues:
- [High] Missing serial port unregistration and timer cleanup on probe error paths causes resource leaks and a Use-After-Free.
- [High] Sysfs attributes are created prematurely in probe() and removed late in remove(), allowing NULL pointer dereference and unmapped memory access.
- [High] Incorrect teardown order in remove() deletes the timer before unregistering the port, allowing the timer to be re-armed.
--

commit 0b81ae92f1a0f52b8cec821070559651477025dd
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

Makes the driver compatible with the ast2600-vuart.
This enables specific configuration for the AST2600.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 26fc0464f1cc2..6afa2f4057e1b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -560,6 +560,7 @@ static void aspeed_vuart_remove(struct platform_device *pdev)
>  static const struct of_device_id aspeed_vuart_table[] = {
>  	{ .compatible = "aspeed,ast2400-vuart" },
>  	{ .compatible = "aspeed,ast2500-vuart" },
> +	{ .compatible = "aspeed,ast2600-vuart" },

[Severity: High]
Does the AST2600 PCI initialization logic bypass the error handling labels
in aspeed_vuart_probe()?

If aspeed_ast2600_vuart_over_pci_set_enabled() fails, the code returns
directly:

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    if (of_device_is_compatible(dev->of_node, "aspeed,ast2600-vuart") &&
        of_property_read_bool(dev->of_node, "aspeed,vuart-over-pci")) {
        rc = aspeed_ast2600_vuart_over_pci_set_enabled(pdev);
        if (rc) {
            dev_err(dev, "could not enable VUART over PCI\n");
            return rc;
        }
    }
    return 0;

err_sysfs_remove:
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    return rc;
}

This direct return bypasses the err_sysfs_remove cleanup label, which would
leave the sysfs group leaked and the UART port and timer active with
pointers to the devm-freed vuart structure.

[Severity: High]
This isn't a bug introduced by this patch, but does aspeed_vuart_probe()
leak resources and cause a use-after-free on other error paths?

If the probe fails and jumps to err_sysfs_remove, it removes the sysfs
group but fails to call serial8250_unregister_port() or timer_delete_sync():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    rc = serial8250_register_8250_port(&port);
    ...
err_sysfs_remove:
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    return rc;
}

Because vuart is devm-allocated, it will be freed upon probe failure,
leaving the UART port registered with private_data pointing to freed memory.

[Severity: High]
This is a pre-existing issue, but are the sysfs attributes exposed too
early in probe() and removed too late in remove()?

In aspeed_vuart_probe(), sysfs_create_group() is called before the
driver data is set via platform_set_drvdata():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_probe() {
    ...
    rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    ...
    platform_set_drvdata(pdev, vuart);
    ...
}

If userspace reads attributes like lpc_address during this window,
dev_get_drvdata() returns NULL which is then dereferenced in the callback.

Additionally, in aspeed_vuart_remove(), sysfs_remove_group() is called
after serial8250_unregister_port():

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_remove() {
    ...
    serial8250_unregister_port(vuart->line);
    sysfs_remove_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
}

Because unregistering the port calls iounmap(), any concurrent sysfs access
in this window could access unmapped I/O memory.

[Severity: High]
This is also a pre-existing issue, but does aspeed_vuart_remove() delete
the unthrottle timer before disabling the interrupt source that re-arms it?

drivers/tty/serial/8250/8250_aspeed_vuart.c:aspeed_vuart_remove() {
    ...
    timer_delete_sync(&vuart->unthrottle_timer);
    aspeed_vuart_set_enabled(vuart, false);
    serial8250_unregister_port(vuart->line);
    ...
}

At the point timer_delete_sync() is called, the UART port is still active.
If an RX interrupt triggers concurrently, aspeed_vuart_handle_irq() can
call mod_timer() and re-arm it. The re-armed timer could then fire after
the devm-allocated vuart structure is freed.

>  	{ },
>  };
>  MODULE_DEVICE_TABLE(of, aspeed_vuart_table);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782224059.git.gregoire.layet@9elements.com?part=3