Re: [PATCH v6 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string

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

Pre-existing issues:
- [High] Probe error paths after 8250 port registration fail to unregister the port, leading to Use-After-Free.
- [High] Sysfs attributes are exposed before driver data is set, leading to a NULL pointer dereference if accessed concurrently.
--

--- Patch [2]: [PATCH v6 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string ---
commit fe8c86639134718c0c590213718a2ba091aac548
Author: Grégoire Layet <[email protected]>

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

Make the driver compatible with the ast2600-vuart.
Enable specific configuration for the AST2600.

> --- 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]
This isn't a bug introduced by this patch, but since this enables the driver
for new hardware, is it possible for the probe error paths to cause a
use-after-free?

Looking at aspeed_vuart_probe() in drivers/tty/serial/8250/8250_aspeed_vuart.c,
the 8250 port is registered early:

aspeed_vuart_probe() {
    ...
    rc = serial8250_register_8250_port(&port);
    if (rc < 0)
        goto err_sysfs_remove;
    ...

If a subsequent step fails, such as the lpc-io-reg property check:

    rc = aspeed_vuart_set_lpc_address(vuart, prop);
    if (rc < 0) {
        dev_err_probe(dev, rc, "invalid value in aspeed,lpc-io-reg property\n");
        goto err_sysfs_remove;
    }

The err_sysfs_remove label returns without calling
serial8250_unregister_port(). Devres will then automatically free the vuart
struct, but the 8250 port remains registered and pointing to freed memory.

If userspace later accesses the port, such as by calling
aspeed_vuart_startup(), would this dereference the freed vuart pointer?


[Severity: High]
This is also a pre-existing issue, but does creating the sysfs group this
early expose a race condition during probe?

In aspeed_vuart_probe(), sysfs attributes are exposed before the driver
data is set:

aspeed_vuart_probe() {
    ...
    rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);
    if (rc < 0)
        return rc;
    ...
    platform_set_drvdata(pdev, vuart);
    return 0;
}

If a userspace process reads a sysfs attribute concurrently before probe
completes, sirq_show() will retrieve a NULL pointer from dev_get_drvdata():

sirq_show() {
    struct aspeed_vuart *vuart = dev_get_drvdata(dev);
    u8 reg;

    reg = aspeed_vuart_readb(vuart, ASPEED_VUART_GCRB);
    ...
}

Will this cause a NULL pointer dereference when aspeed_vuart_readb()
attempts to access vuart->port?

>  	{ },
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2