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

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

On Thu, 21 May 2026 23:52:56 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential
> issue(s) to consider:
> 
> New issues:
> - [High] vmd_resume() unconditionally modifies legacy MSI remapping
> configuration on unsupported hardware.
Ack.
> 
> Pre-existing issues:
> - [High] SRCU structures are permanently leaked on probe error paths.
> - [High] SRCU structs are destroyed while the managed IRQ handler is
> still registered, leading to a Use-After-Free.
> - [High] Truncation of 64-bit physical addresses on 32-bit systems
> due to incorrect use of PCI_BASE_ADDRESS_MEM_MASK. --

Proposing to fix these issues in separate patches to keep the patch
size more manageable.
> 
> commit f05a884975712ad2dc4d55677318e1620e041df9
> 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.
> 
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c index d4ae250d4bc6d..15a3f6a357889
> > 100644 --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c  
> [ ... ]
> > @@ -661,6 +684,46 @@ static int vmd_get_bus_number_start(struct
> > vmd_dev *vmd) return 0;
> >  }
> >  
> > +static int vmd_get_bus_info_from_bar4(struct vmd_dev *vmd,
> > +				       resource_size_t *offset1,
> > +				       resource_size_t *offset2)
> > +{  
> [ ... ]
> > +	/* Calculate offsets like vmd_get_phys_offsets() does. */
> > +	if (phys1)
> > +		*offset1 = vmd->dev->resource[VMD_MEMBAR1].start -
> > +			(phys1 & PCI_BASE_ADDRESS_MEM_MASK);
> > +	if (phys2)
> > +		*offset2 = vmd->dev->resource[VMD_MEMBAR2].start -
> > +			(phys2 & PCI_BASE_ADDRESS_MEM_MASK);  
> 
> [Severity: High]
> Will this truncate the 64-bit physical addresses on 32-bit systems?
> 
> Since PCI_BASE_ADDRESS_MEM_MASK is ~0x0fUL, on 32-bit systems it
> evaluates to a 32-bit 0xFFFFFFF0. When bitwise ANDed with a 64-bit
> u64 like phys1 or phys2, the mask is zero-extended to
> 0x00000000FFFFFFF0ULL, clearing the upper 32 bits of the address.
> 
> Could this result in entirely incorrect memory mapping offsets for
> VMD devices mapped above 4GB?
> 
> This is a pre-existing issue in vmd_get_phys_offsets() as well, but
> it seems to be repeated here.
> 
> > +
> > +	pci_iounmap(vmd->dev, bar4);
> > +	return 0;
> > +}
> > +
> >  static irqreturn_t vmd_irq(int irq, void *data)
> >  {
> >  	struct vmd_irq_list *irqs = data;
> > @@ -711,6 +774,53 @@ static int vmd_alloc_irqs(struct vmd_dev *vmd)
> >  	return 0;
> >  }  
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the driver leak
> srcu_struct structures on the probe error paths?
> 
> Looking at vmd_alloc_irqs(), it initializes srcu_struct structures in
> a loop. If a subsequent step fails (such as init_srcu_struct or
> vmd_create_irq_domain later in probe), vmd_probe() jumps to
> out_release_instance and returns an error without calling
> cleanup_srcu_struct() for the already initialized structures.
> 
> While IRQ vectors are automatically freed via devres, the srcu_struct
> allocations inside the per-CPU memory appear to be permanently leaked.
> 
> [Severity: High]
> This is also a pre-existing issue, but does vmd_remove() introduce a
> use-after-free by manually destroying srcu_struct structures while
> the managed IRQ handler is still registered?
> 
> The driver registers its interrupt handlers using devm_request_irq().
> Because devres handlers run only after vmd_remove() returns, the IRQ
> remains active and can fire during or after the srcu_struct
> structures are manually destroyed.
> 
> If vmd_irq() fires during this window, it calls srcu_read_lock() on a
> destroyed srcu_struct. Can this sequence trigger a use-after-free?
> 
> [ ... ]
> > @@ -868,19 +956,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;
> >  
> > -		vmd_set_msi_remapping(vmd, true);
> > +			vmd_set_msi_remapping(vmd, true);  
> 
> [Severity: High]
> This patch bypasses MSI remapping initialization for
> VMD_FEAT_USE_BIOS_INFO in vmd_enable_domain(), but is it also
> necessary to update vmd_resume()?
> 
> vmd_resume() unconditionally calls vmd_set_msi_remapping(), which
> will read and write the legacy PCI_REG_VMCONFIG register during
> system resume. Could this cause undefined behavior or PCI aborts on
> the new hardware?
> 

 I will address this issue. We dont want vmd_resume to ser msi for
 unsupported devices.