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

[Nirmal Patel <nirmal.patel@linux.intel.com>]

On Tue, 19 May 2026 16:05:04 +0000
sashiko-bot@kernel.org wrote:

> 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?
No. This is for vmd driver to read and assign bus start range and
location of shadow registers.
> 
> 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.
> 

These two hardware registers don't have dependency. BASE_ID_REG_28C1
holds bus start value. Since the hardware register is named as Base ID,
I kept the name as it is.

> 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?
There is no overlap. All of the these registers are separate.
64-bit SHADOW_MEMBAR1_28C1 and SHADOW_MEMBAR2_28C1 are located at
0x2818 and 0x2820. So the 16 byte size MB2_SHADOW_SIZE includes both of
these registers.
The BASE_ID_REG_28C1 is located at 0x2840, much farther from shadow
registers.
> 
> > +			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?

resource 4 is needed to obtain required information for bus scan. So
unassigned resource 4 should return an error and stop the enumeration/
probe. I can return an error in else part.

> 
> > +	} 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?

This is a good point. But in my v1 patch, I kept vmd->busn_start +
(resource_size(res) >> 20) - 1. it was flagged as an issue.
If the busn_start is 128 and ConfigBar is 256 MB, then .end will be set
to out of bound value. I can set hard coded value of 0xff same as
pci/probe.c

> 
> >  		.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?

I can address this issue in another patch series as a bug fix or
improvement. 

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