Re: [PATCH v4 4/7] serial: 8250_aspeed_vuart: add VUART over PCI

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Probe error paths fail to unregister the 8250 serial port and cancel the unthrottle timer, causing a resource leak and potential use-after-free.
- [High] Sysfs attributes are exposed before the driver data is set, creating a race window that leads to a NULL pointer dereference.
--

--- Patch [4]: [PATCH v4 4/7] serial: 8250_aspeed_vuart: add VUART over PCI ---
commit d4ec19577207abb14c5ca6354d48ef6da9508b14
Author: Grégoire Layet <gregoire.layet@9elements.com>

serial: 8250_aspeed_vuart: add VUART over PCI

Enable the VUART over PCI for the AST2600. Activate it only if the
'aspeed,vuart-over-pci' property flag is set on an 'ast2600-vuart'
compatible node.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 6afa2f4057e1b..4d09c04cb9727 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c

[ ... ]

> @@ -540,6 +617,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);
>  	platform_set_drvdata(pdev, vuart);

[Severity: High]
This is a pre-existing issue, but setting the driver data this late creates a
race condition.

The sysfs attribute group is created earlier in aspeed_vuart_probe():

    rc = sysfs_create_group(&vuart->dev->kobj, &aspeed_vuart_attr_group);

If userspace accesses these sysfs files (e.g., triggering sirq_show()) before
platform_set_drvdata() is called here, dev_get_drvdata() returns NULL, which
then leads to a NULL pointer dereference.

Could the sysfs creation be deferred until after the driver data is set?

>  
> +	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 < 0) {
> +			dev_err_probe(dev, rc, "could not enable VUART over PCI\n");
> +			goto err_sysfs_remove;

[Severity: High]
This is a pre-existing issue, but this error path fails to unregister the 8250
serial port and cancel the unthrottle timer.

If aspeed_ast2600_vuart_over_pci_set_enabled() fails, we jump to
err_sysfs_remove:

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

This cleans up the sysfs group but leaves the serial port registered.

Since the vuart structure is allocated with devm_kzalloc, it will be freed
automatically by devres when the probe fails. This leaves a dangling pointer
in the active 8250 serial port's private data, which can result in a
use-after-free if the TTY device is accessed.

Should this error path also call serial8250_unregister_port() and
timer_delete_sync() before returning?

> +		}
> +	}
> +
>  	return 0;
>  
>  err_sysfs_remove:

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