Re: [PATCH v5 3/6] serial: 8250_aspeed_vuart: add VUART over PCI

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

New issues:
- [Low] The FIELD_PREP macro is used without explicitly including <linux/bitfield.h>.

Pre-existing issues:
- [High] Probe error paths fail to unregister the 8250 serial port and delete the unthrottle timer, leading to a Use-After-Free when devres frees the device memory.
- [High] Incorrect timer deletion ordering in `aspeed_vuart_remove()` allows the timer to be re-armed on a freed object.
--

--- Patch [3]: [PATCH v5 3/6] serial: 8250_aspeed_vuart: add VUART over PCI ---
commit ff31cbc0cdc941ba791069ea924a94d8ad864389
Author: Grégoire Layet <[email protected]>

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..2360d37139bfa 100644
> --- a/drivers/tty/serial/8250/8250_aspeed_vuart.c
> +++ b/drivers/tty/serial/8250/8250_aspeed_vuart.c
> @@ -32,6 +32,28 @@
>  #define ASPEED_VUART_DEFAULT_SIRQ	4
>  #define ASPEED_VUART_DEFAULT_SIRQ_POLARITY	IRQ_TYPE_LEVEL_LOW
>  
> +#define ASPEED_SCU_SILICON_REVISION_ID			0x04
> +#define AST2600A3_REVISION_ID				0x05030303
> +
> +#define ASPEED_SCU_PCIE_CONF_CTRL 0xC20

[ ... ]

> +#define ASPEED_SCUC24 0xC24
> +#define  ASPEED_SCUC24_MSI_ROUTING_MASK				GENMASK(11, 10)
> +#define  ASPEED_SCUC24_MSI_ROUTING_PCIE2LPC_PCIDEV1 \
> +		FIELD_PREP(ASPEED_SCUC24_MSI_ROUTING_MASK, 2)

[Severity: Low]
Does this file explicitly include <linux/bitfield.h> for the FIELD_PREP
macro? It appears missing, which might cause build failures if implicit
inclusions change in the future.

> +#define  ASPEED_SCUC24_RESERVED_14					BIT(14)
> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_SCU560_EN	BIT(17)
> +#define  ASPEED_SCUC24_PCIDEV1_INTX_MSI_HOST2BMC_EN	BIT(18)

[ ... ]

> @@ -540,6 +622,15 @@ static int aspeed_vuart_probe(struct platform_device *pdev)
>  	aspeed_vuart_set_host_tx_discard(vuart, true);
>  	platform_set_drvdata(pdev, vuart);
>  
> +	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 adding this new error path exposes it
further in aspeed_vuart_probe().

If we jump to err_sysfs_remove after the UART port has been registered via
serial8250_register_8250_port(), does this path fail to unregister the port
and delete the unthrottle timer?

Since the vuart structure is allocated with devm_kzalloc, returning an error
here will cause devres to free the memory. Could this leave the 8250
subsystem with a registered port containing a dangling pointer to vuart in
port.private_data?

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

While reviewing the lifecycle of the vuart object, I also noticed another
related issue in aspeed_vuart_remove() which is not modified in this patch.

[Severity: High]
This isn't a bug introduced by this patch, but does the ordering in
aspeed_vuart_remove() allow the unthrottle timer to be re-armed on a freed
object?

Currently, timer_delete_sync(&vuart->unthrottle_timer) is called before
serial8250_unregister_port(vuart->line). Since the serial port and its
interrupt handler remain active during this window, could an interrupt arrive
and trigger aspeed_vuart_handle_irq(), calling mod_timer() to re-arm the
timer? 

When aspeed_vuart_remove() completes, vuart is freed by devres, and the
pending timer could subsequently execute and access the freed memory.

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