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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe error paths leak the registered 8250 port and timer, leading to a use-after-free when devres cleans up.
- [High] Early sysfs exposure in probe allows userspace to trigger a NULL pointer dereference before drvdata is set.
- [High] Late sysfs removal during device remove allows userspace to access unmapped IO memory.
--

--- Patch [2]: [PATCH v5 2/6] serial: 8250_aspeed_vuart: add aspeed,ast2600-vuart compatible string ---
commit 513026908b64cd69b7d61f618f21a1466256a146
Author: Grégoire Layet <gregoire.layet@9elements.com>

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

Make the driver compatible with the ast2600-vuart.
Enable 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]
This isn't a bug introduced by this patch, but adding this compatible string
exposes the AST2600 device to a pre-existing resource leak in the driver's
probe error path:

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

The err_sysfs_remove error path fails to call serial8250_unregister_port()
and timer_delete_sync() before returning an error, leaking resources that hold
a dangling pointer to the devres-freed vuart structure.

Could this lead to a use-after-free when devres cleans up?

[Severity: High]
This is also a pre-existing issue, but there is a race condition with early
sysfs exposure during probe:

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);
    ...
}

The sysfs group is created and exposed to userspace early in the probe
function, creating a race window before platform_set_drvdata() is called at
the end.

If userspace reads a sysfs attribute (like sirq) during this window, the
handler will call dev_get_drvdata() and receive NULL. Can this allow
userspace to trigger a NULL pointer dereference before drvdata is set?

[Severity: High]
This is a pre-existing issue as well, but there appears to be a use-after-unmap
vulnerability in the remove function:

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);
}

The underlying 8250 port is unregistered and unmapped before the sysfs
interface is removed.

If userspace accesses a sysfs attribute concurrently in the window before the
sysfs group is removed, can this result in an access violation on the now
unmapped vuart->port->port.membase?

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

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