Re: [PATCH v3 5/7] soc: aspeed: add host-side PCIe BMC device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Modifying a shared static global array on IRQ allocation fallback permanently corrupts the driver state for all subsequent devices and creates a data race.
- [High] The driver mishandles `pci_alloc_irq_vectors()` return values, leading to silent probe successes with invalid IRQs or out-of-bounds vector accesses that trigger kernel warnings.
- [Medium] Truncation of a 64-bit PCI physical address due to storing it in an `unsigned long` on 32-bit architectures.
--

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

soc: aspeed: add host-side PCIe BMC device driver

Add support for VUART over PCIe between BMC and host.
This configures the PCIe and sets up two 8250 compatible VUART ports.

> diff --git a/drivers/soc/aspeed/aspeed-host-bmc-dev.c b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
> new file mode 100644
> index 0000000000000..3160b6aedb5b6
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-host-bmc-dev.c
[ ... ]
> +static int vuart_msi_index[2] = { 16, 17 };
> +static int vuart_port_addr[2] = {0x3f8, 0x2f8};
> +
> +struct aspeed_pci_bmc_dev {
> +	struct device *dev;
> +	kernel_ulong_t driver_data;
> +	int id;
> +
> +	unsigned long message_bar_base;

[Severity: Medium]
Does storing the BAR address in an unsigned long truncate 64-bit physical
addresses on 32-bit architectures with PAE/LPAE?

pci_resource_start() returns a resource_size_t, which could be 64-bit,
potentially leading to incorrect I/O memory mapping here.

> +
> +	struct uart_8250_port uart[2];
> +	int uart_line[2];
> +
> +	int *msi_idx_table;
> +};
> +
> +static void aspeed_pci_setup_irq_resource(struct pci_dev *pdev)
> +{
> +	struct aspeed_pci_bmc_dev *pci_bmc_dev = pci_get_drvdata(pdev);
> +
> +	pci_bmc_dev->msi_idx_table = vuart_msi_index;
> +
> +	if (pci_alloc_irq_vectors(pdev, 1, BMC_MULTI_MSI, PCI_IRQ_INTX | PCI_IRQ_MSI) <= 1)

[Severity: High]
Can this condition mishandle partial allocations and negative error codes?

If pci_alloc_irq_vectors() partially succeeds and returns a value between
2 and 16, the <= 1 check evaluates to false. This leaves the indices at 16
and 17, but pci_irq_vector() will later trigger a WARN_ON_ONCE out-of-bounds
check in the PCI core when it tries to access index 16.

Additionally, if it fails entirely and returns a negative error, the check
passes, but the probe doesn't abort. Does this result in silently registering
the 8250 UART with an invalid IRQ?

> +		/* If pci_alloc fail, set all msi index to the first vector */
> +		memset(pci_bmc_dev->msi_idx_table, 0, sizeof(vuart_msi_index));

[Severity: High]
Does this memset corrupt the driver state for all subsequent instances?

Because pci_bmc_dev->msi_idx_table points directly to the static global array
vuart_msi_index, writing zeroes here permanently overwrites the shared array.

If multiple ASPEED PCI devices are probed, or if the device is probed again
later, will the indices be permanently stuck at 0?

> +}

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