Re: [PATCH v5 7/8] PCI: vmd: Add support for second rootbus under VMD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Flawed arithmetic when configuring MEMBAR offsets guarantees overlapping or gapped PCI resource windows between BUS0 and BUS1.
- [High] Failure to clean up VMD_BUS_0 on VMD_BUS_1 creation failure leads to a severe Use-After-Free.
- [High] Iterating over an unvalidated sum of disjoint bus ranges in `vmd_domain_reset` causes missed bus resets and potential out-of-bounds MMIO accesses.
- [Medium] VMD_RES_BUS1_CFGBAR is assigned an IORESOURCE_MEM resource as its parent, corrupting the resource tree.
- [Medium] An uninitialized stack variable is assigned to resource bounds if PCI config space reads fail.
--

commit 46d85add5d9d3ed310b9c5a3c11026a4231b01a9
Author: Szymon Durawa <szymon.durawa@linux.intel.com>
Subject: PCI: vmd: Add support for second rootbus under VMD

Starting from Intel Arrow Lake VMD enhancement introduces second root bus
support with fixed root bus number (0x80). It means that all 3 MMIO BARs
exposed by VMD are shared now between both buses (current BUS0 and
new BUS1).

Add new BUS1 enumeration and divide MMIO space to be shared between
both root buses.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 2bfc16875d912..c68ea7837620d 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c

[ ... ]

> @@ -537,11 +568,23 @@ static inline void vmd_acpi_end(void) { }
>  
>  static void vmd_domain_reset(struct vmd_dev *vmd)
>  {
> -	u16 bus, max_buses = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
> +	u16 bus, bus_cnt = resource_size(&vmd->resources[VMD_RES_CFGBAR]);
>  	u8 dev, functions, fn, hdr_type;
>  	char __iomem *base;
>  
> -	for (bus = 0; bus < max_buses; bus++) {
> +	/*
> +	 * Total bus-number count (bus_cnt) when BUS1 root bus is present:
> +	 * initialized with BUS0 CFGBAR range, extended by BUS1 CFGBAR range plus
> +	 * one bus number per root bus' own config space (bus_cnt is increased by 2)
> +	 * as the CFGBAR ranges do not include the root bus config space.
> +	 */
> +
> +	if (vmd->bus1_rootbus) {
> +		bus_cnt += resource_size(&vmd->resources[VMD_RES_BUS1_CFGBAR]);
> +		bus_cnt += 2;
> +	}
> +
> +	for (bus = 0; bus < bus_cnt; bus++) {
>  		for (dev = 0; dev < 32; dev++) {
>  			base = vmd->cfgbar + PCIE_ECAM_OFFSET(bus,
>  						PCI_DEVFN(dev, 0), 0);

[Severity: High]
The bus_cnt variable combines the sizes of both the BUS0 and BUS1 CFGBAR
ranges. If the ranges for BUS0 and BUS1 are sparse, does this continuous
loop up to the sum of their sizes end up missing the higher bus numbers?

Could this also access out-of-bounds MMIO offsets if the sum exceeds the
mapped region for ECAM?

[ ... ]

> @@ -799,6 +867,30 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>  		       (resource_size(res) >> 20) - 1,
>  		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
>  	};
> +
> +	if (vmd->bus1_rootbus) {
> +		u16 bus0_range = 0;
> +		u16 bus1_range = 0;
> +
> +		pci_read_config_word(vmd->dev, VMD_BUSRANGE0, &bus0_range);
> +		pci_read_config_word(vmd->dev, VMD_BUSRANGE1, &bus1_range);
> +
> +		/*
> +		 * Resize BUS0 CFGBAR range to make space for BUS1
> +		 * owned devices by adjusting range end with value stored in
> +		 * VMD_BUSRANGE0 register.
> +		 */
> +		vmd->resources[VMD_RES_CFGBAR].start = VMD_BUS_START(bus0_range);
> +		vmd->resources[VMD_RES_CFGBAR].end = VMD_BUS_END(bus0_range);
> +
> +		vmd->resources[VMD_RES_BUS1_CFGBAR] = (struct resource){
> +			.name = "VMD CFGBAR BUS1",
> +			.start = VMD_BUS_START(bus1_range),
> +			.end = VMD_BUS_END(bus1_range),
> +			.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
> +			.parent = res,
> +		};
> +	}
>  }

[Severity: Medium]
The res pointer here refers to VMD_CFGBAR, which is an IORESOURCE_MEM
resource. Is it safe to assign a memory resource as the parent to
VMD_RES_BUS1_CFGBAR, which is an IORESOURCE_BUS resource?

Will this cross-namespace parenting confuse the generic PCI resource
management?

[ ... ]

> @@ -840,41 +932,83 @@ static void vmd_configure_membar(struct vmd_dev *vmd,
>  static void vmd_configure_membar1_membar2(struct vmd_dev *vmd,
>  					  resource_size_t mbar2_ofs)
>  {
> -	vmd_configure_membar(vmd, 1, VMD_MEMBAR1, 0, 0);
> -	vmd_configure_membar(vmd, 2, VMD_MEMBAR2, mbar2_ofs, 0);
> +	if (vmd->bus1_rootbus) {
> +		u32 bus1_mbar1_ofs = 0;
> +		u64 bus1_mbar2_ofs = 0;
> +		u32 reg;
> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR1_OFFSET,
> +				      &bus1_mbar1_ofs);
> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET1, &reg);
> +		bus1_mbar2_ofs = reg;

[Severity: Medium]
If pci_read_config_dword() fails due to a bus error or unsupported
operation, it leaves reg untouched. Could this result in uninitialized
stack memory being used to configure the physical MMIO allocation for
BUS1?

> +
> +		pci_read_config_dword(vmd->dev, VMD_MEMBAR2_OFFSET2, &reg);
> +		bus1_mbar2_ofs |= (u64)reg << 32;
> +
> +		/*
> +		 * Resize BUS MEMBAR1 and MEMBAR2 ranges to make space
> +		 * for BUS1 owned devices by adjusting range end with values
> +		 * stored in VMD_MEMBAR1_OFFSET and VMD_MEMBAR2_OFFSET registers
> +		 */
> +		vmd_configure_membar(vmd, VMD_RES_MBAR_1, VMD_MEMBAR1, 0,
> +				     bus1_mbar1_ofs);
> +		vmd_configure_membar(vmd, VMD_RES_MBAR_2, VMD_MEMBAR2,
> +				     mbar2_ofs, bus1_mbar2_ofs - mbar2_ofs);
> +
> +		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_1, VMD_MEMBAR1,
> +				     bus1_mbar1_ofs, 0);
> +		vmd_configure_membar(vmd, VMD_RES_BUS1_MBAR_2, VMD_MEMBAR2,
> +				     mbar2_ofs + bus1_mbar2_ofs, 0);

[Severity: Critical]
The vmd_configure_membar() function subtracts the last argument from the
end of the resource. By passing bus1_mbar1_ofs as both the start offset
for BUS1 and the amount to subtract from the end of BUS0, won't the
resources overlap?

For example, if the offset is smaller than half the total BAR size, does
this cause identical physical MMIO ranges to be assigned to distinct
devices on BUS0 and BUS1?

[ ... ]

> @@ -1028,6 +1164,27 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  
>  	vmd_bus_enumeration(vmd->bus[VMD_BUS_0], features);
>  
> +	if (vmd->bus1_rootbus) {
> +		ret = vmd_create_bus(vmd, VMD_BUS_1, sd, offset);
> +		if (ret) {
> +			pci_err(vmd->dev, "Can't create BUS1: %d\n", ret);
> +			return ret;
> +		}

[Severity: High]
If vmd_create_bus() fails for VMD_BUS_1, this path returns early. However,
VMD_BUS_0 has already been fully created and enumerated earlier in this
function.

Since vmd_remove() is not called on the probe error path, does this leave
VMD_BUS_0 active while the struct vmd_dev context is subsequently freed by
devres, resulting in a use-after-free?

> +
> +		/*
> +		 * Primary bus is not set by pci_create_root_bus(), it is
> +		 * updated here
> +		 */
> +		vmd->bus[VMD_BUS_1]->primary = VMD_PRIMARY_BUS1;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=7