Re: [PATCH v2] PCI: vmd: Add feature to scan BIOS enumerated devices.

[sashiko-bot@kernel.org]

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

New issues:
- [High] Invalid VMD CFGBAR resource range calculated for older hardware with restricted bus ranges.
- [High] VMD shadow register exposed to the child device resource pool, risking memory corruption.
- [Medium] Silent failure and continued execution with uninitialized variables if BAR4 is not an IORESOURCE_MEM.

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() calls on error paths leaks IRQ resources.
--

commit fb1f4c20650569f7ba38772b55e3d4757083bcf4
Author: Nirmal Patel <nirmal.patel@linux.intel.com>

PCI: vmd: Add feature to scan BIOS enumerated devices.

Newer VMD with device ID 0x28c1 has unique settings compared to its
predecessor where BIOS enumerates the entire VMD device tree and
assigns respective configurations. This patch adds support to obtain
the bus hide range and shadow registers set by BIOS.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index d4ae250d4bc6d..ca8738285659a 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
[ ... ]
> @@ -711,6 +773,55 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
>  	return 0;
>  }
>  
> +static int vmd_prepare_offsets_and_bus(struct vmd_dev *vmd,
> +					unsigned long features,
> +					resource_size_t *membar2_offset,
> +					resource_size_t *offset1,
> +					resource_size_t *offset2)
> +{
> +	int ret;
[ ... ]
> +	if (features & VMD_FEAT_USE_BIOS_INFO) {
> +		if (resource_type(&vmd->dev->resource[4]) == IORESOURCE_MEM) {
> +			*membar2_offset = SHADOW_MEMBAR1_28C1 + MB2_SHADOW_SIZE;

[Severity: High]
Does this offset calculation expose VMD configuration registers to child
devices?

The calculation SHADOW_MEMBAR1_28C1 + MB2_SHADOW_SIZE resolves to 0x2828.
However, the BASE_ID_REG_28C1 register is at offset 0x2840, which falls
after 0x2828.

If a child device maps a BAR over this region, could it overwrite the
VMD shadow register at 0x2840 and cause hardware misbehavior?

Should the offset be set large enough to protect all internal VMD registers,
such as >= 0x2848?

> +			ret = vmd_get_bus_info_from_bar4(vmd, offset1, offset2);
> +			if (ret)
> +				return ret;
> +		}

[Severity: Medium]
What happens if VMD_FEAT_USE_BIOS_INFO is set, but resource 4 is not
of type IORESOURCE_MEM?

Because the subsequent fallback checks use else if, they are bypassed
entirely. The function will silently return 0 without retrieving valid
offsets.

Will this cause the driver to probe with uninitialized offset values,
failing to handle broken hardware or firmware configurations?

> +	} else if (features & VMD_FEAT_HAS_MEMBAR_SHADOW) {
> +		*membar2_offset = MB2_SHADOW_OFFSET + MB2_SHADOW_SIZE;
> +		ret = vmd_get_phys_offsets(vmd, true, offset1, offset2);
[ ... ]
> @@ -784,38 +895,16 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	struct pci_dev *dev;
>  	int ret;
[ ... ]
> -	if (features & VMD_FEAT_HAS_BUS_RESTRICTIONS) {
> -		ret = vmd_get_bus_number_start(vmd);
> -		if (ret)
> -			return ret;
> -	}
> +	ret = vmd_prepare_offsets_and_bus(vmd, features, &membar2_offset,
> +					  &offset[0], &offset[1]);
> +	if (ret)
> +		return ret;
>  
>  	res = &vmd->dev->resource[VMD_CFGBAR];
>  	vmd->resources[0] = (struct resource) {
>  		.name  = "VMD CFGBAR",
>  		.start = vmd->busn_start,
> -		.end   = vmd->busn_start + (resource_size(res) >> 20) - 1,
> +		.end   = (resource_size(res) >> 20) - 1,

[Severity: High]
Does this change break bus enumeration for older hardware with restricted bus
ranges?

By unconditionally dropping vmd->busn_start from the .end calculation,
it works for the new 28C1 device with a busn_start of 0.

However, for an older device with busn_start = 128 and a 32 MB CFGBAR
size, .start would be 128, but .end would calculate to 31.

Could this create a malformed resource where .start > .end?

>  		.flags = IORESOURCE_BUS | IORESOURCE_PCI_FIXED,
>  	};
[ ... ]
> @@ -868,19 +957,21 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  	 * acceptable because the guest is usually CPU-limited and MSI
>  	 * remapping doesn't become a performance bottleneck.
>  	 */
> -	if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> -	    offset[0] || offset[1]) {
> -		ret = vmd_alloc_irqs(vmd);
> -		if (ret)
> -			return ret;
> +	if (!(features & VMD_FEAT_USE_BIOS_INFO)) {
> +		if (!(features & VMD_FEAT_CAN_BYPASS_MSI_REMAP) ||
> +		    offset[0] || offset[1]) {
> +			ret = vmd_alloc_irqs(vmd);
> +			if (ret)
> +				return ret;

[Severity: High]
This is a pre-existing issue, but does this error path leak IRQ resources?

According to PCI subsystem guidelines, every error path after a successful
pci_alloc_irq_vectors() must call pci_free_irq_vectors() before
returning.

If vmd_enable_domain() fails after allocating IRQs, or if
vmd_alloc_irqs() fails internally (e.g., during devm_kcalloc(),
init_srcu_struct(), or devm_request_irq()), it appears to return
directly without freeing the allocated MSI-X vectors.

Could this eventually exhaust system IRQ capacity?

> +			vmd_set_msi_remapping(vmd, true);
>  
> -		ret = vmd_create_irq_domain(vmd);
> -		if (ret)
> -			return ret;
> -	} else {
> -		vmd_set_msi_remapping(vmd, false);
> +			ret = vmd_create_irq_domain(vmd);
> +			if (ret)
> +				return ret;
> +		} else {
> +			vmd_set_msi_remapping(vmd, false);
> +		}
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519151832.9005-1-nirmal.patel@linux.intel.com?part=1