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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The newly added error path directly returns on failure, bypassing sysfs cleanup and leaving the hardware enabled and port registered.
- [Medium] Brittle lookup of the SCU syscon regmap by parsing the "clocks" property.
- [Medium] Setting untested and reserved bits in SCU registers.

Pre-existing issues:
- [High] Preexisting error paths fail to unregister the 8250 serial port and delete the timer on probe failure, leading to a Use-After-Free.
- [High] Unprotected Read-Modify-Write operations on shared hardware registers.
- [High] Sysfs files and TTY port are exposed to userspace before driver data is initialized.
--

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

    serial: 8250_aspeed_vuart: add VUART over PCI

This commit adds support for VUART over PCI on the AST2600 platform. It
configures the BMC PCI device enables, sets the PCI class code to MFD, and
configures MSI interrupts based on the silicon revision.

> diff --git a/drivers/tty/serial/8250/8250_aspeed_vuart.c b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> index 6afa2f4057e1b..e204e26fa173b 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
[ ... ]
> @@ -412,6 +433,62 @@ static int aspeed_vuart_map_irq_polarity(u32 dt)
> +static int aspeed_ast2600_vuart_over_pci_set_enabled(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 silicon_revision_id;
> +	struct regmap *scu;
> +	int rc;
> +
> +	u32 pcie_config_ctl = SCU_PCIE_CONF_BMC_DEV_EN_IRQ |
> +						SCU_PCIE_CONF_BMC_DEV_EN_MMIO |
> +						SCU_PCIE_CONF_BMC_DEV_EN_MSI |
> +						SCU_PCIE_CONF_BMC_DEV_EN_PCIE_BUS_MASTER |
> +						SCU_PCIE_CONF_BMC_DEV_EN_E2L |
> +						SCU_PCIE_CONF_BMC_DEV_EN_LPC_DECODE |
> +						SCU_PCIE_CONF_BMC_DEV_EN;
> +
> +	scu = syscon_regmap_lookup_by_phandle(dev->of_node, "clocks");
> +	if (IS_ERR(scu)) {

[Severity: Medium]
Will this break if any clock multiplexer or intermediate clock controller is
inserted between the SCU and the VUART?

This code uses syscon_regmap_lookup_by_phandle() to fetch the SCU regmap by
parsing the "clocks" property. While this may happen to work if the SCU is
the direct clock provider, does this conflate the clock provider bindings
with syscon mappings?

> +		dev_err(&pdev->dev, "failed to find SCU regmap\n");
> +		return PTR_ERR(scu);
> +	}
[ ... ]
> +	if (silicon_revision_id == AST2600A3_REVISION_ID)
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_MASK,
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
> +	else
> +		rc = regmap_update_bits(scu, ASPEED_SCUC24,
> +					/* The bit 14 is reserved in the Datasheet, so we can't say what it does. This revision has not been tested */
> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_MASK,

[Severity: Medium]
Is it safe to write BIT(14) into ASPEED_SCUC24 when it is undocumented and
reserved?

The comment indicates the bit's purpose is unknown and this revision is
untested. Can setting this undocumented, reserved bit trigger unpredictable
hardware behavior on other silicon revisions?

> +				   ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN | BIT(14) | ASPEED_SCUC24_MSI_ROUTING_PCIe2LPC_PCIDEV1);
> +	if (rc) {
> +		dev_err(dev, "could not set PCI device 1 MSI interrupt routing\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -540,6 +617,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent access
clobber register modifications?

Functions like aspeed_vuart_set_host_tx_discard() and
aspeed_vuart_set_sirq_polarity() perform read-modify-write operations on
ASPEED_VUART_GCRA. Does a concurrent sysfs write and TTY port open/close
cause a race condition here, since these are invoked from contexts with
different locks (or no locks)?

>  	platform_set_drvdata(pdev, vuart);

[Severity: High]
This isn't a bug introduced by this patch, but does the delay in calling
platform_set_drvdata() expose userspace to a null pointer dereference?

In aspeed_vuart_probe(), sysfs_create_group() is called very early.
If a concurrent userspace process reads the sysfs files, invoking
lpc_address_show() before platform_set_drvdata() is called here, it will
retrieve a null pointer and immediately dereference it.

>  
> +	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;

[Severity: High]
Does this direct return bypass necessary cleanup logic and leave the
hardware enabled?

By returning rc directly instead of jumping to a cleanup label, the sysfs
group aspeed_vuart_attr_group is leaked. Furthermore, does this bypass
unregistering the 8250 port and leave the VUART active despite probe
failure, since aspeed_vuart_set_enabled(vuart, true) was called earlier?

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

[Severity: High]
This isn't a bug introduced by this patch, but does this error path lead
to a use-after-free?

The err_sysfs_remove block cleans up sysfs attributes but entirely omits
calling serial8250_unregister_port() and timer_delete_sync(). Because the
driver uses devres, the vuart structure will be freed upon probe failure.
Since the port remains actively registered, any subsequent access to the
port via TTY operations will dereference this freed memory.

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