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

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

On Mon, 22 Jun 2026 15:56:31 +0000
sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider:
> - [Critical] vmd_remove_irq_domain() returns early for devices with
> VMD_FEAT_USE_BIOS_INFO, permanently leaking the IRQ domain and its
> fwnode, and causing an exploitable Use-After-Free.
> - [High] vmd_domain_reset() accesses incorrect ECAM offsets when
> VMD_FEAT_USE_BIOS_INFO is set, failing to reset actual VMD bridges
> and corrupting unrelated MMIO configuration space.
> - [Medium] The new `vmd_get_bus_info_from_bar4()` function accesses
> MMIO regions in BAR4 at hardcoded offsets without validating that the
> mapped BAR is sufficiently large. --
> 
> commit 371d26de7e81b3dcfea77481a03dd53acb14c51c
> Author: Nirmal Patel <nirmal.patel@linux.intel.com>
> 
> PCI: vmd: Add feature to scan BIOS enumerated devices.
> 
> Newer VMD devices with ID 0x28c1 use BIOS enumeration for the device
> tree. This commit updates the VMD driver to read bus hide ranges and
> shadow registers from VMD BAR4 to perform bus scans for these devices.
> 
> > diff --git a/drivers/pci/controller/vmd.c
> > b/drivers/pci/controller/vmd.c index d4ae250d4bc6d..a4e03f10a54d6
> > 100644 --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c  
> 
> [ ... ]
> 
> > @@ -374,6 +393,9 @@ static void vmd_set_msi_remapping(struct
> > vmd_dev *vmd, bool enable) 
> >  static void vmd_remove_irq_domain(struct vmd_dev *vmd)
> >  {
> > +	if (!!(vmd->features & VMD_FEAT_USE_BIOS_INFO))
> > +		return;
> > +  
> 
> [Severity: Critical]
> Does this early return bypass irq_domain_remove() and
> irq_domain_free_fwnode(), permanently leaking the IRQ domain?
> 
> In vmd_enable_domain(), vmd_create_irq_domain() is successfully called
> for these devices because VMD_FEAT_CAN_BYPASS_MSI_REMAP is not set.
> 
> If the IRQ domain remains globally registered, could it cause a
> use-after-free when its struct vmd_dev host_data pointer is
> automatically freed by devres during device removal?
Let me fix it.
> 
> >  	/*
> >  	 * Some production BIOS won't enable remapping between
> > soft reboots.
> >  	 * Ensure remapping is restored before unloading the
> > driver.  
> 
> [ ... ]
> 
> > @@ -393,7 +415,12 @@ static void __iomem *vmd_cfg_addr(struct
> > vmd_dev *vmd, struct pci_bus *bus, unsigned int devfn, int reg, int
> > len) {
> >  	unsigned int busnr_ecam = bus->number - vmd->busn_start;
> > -	u32 offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);
> > +	u32 offset;
> > +
> > +	if (!!(vmd->features & VMD_FEAT_USE_BIOS_INFO))
> > +		busnr_ecam = bus->number;
> > +
> > +	offset = PCIE_ECAM_OFFSET(busnr_ecam, devfn, reg);  
> 
> [Severity: High]
> Since the ECAM offset calculation now uses the absolute bus number for
> VMD_FEAT_USE_BIOS_INFO devices, does vmd_domain_reset() also need to
> be updated?
> 
> Looking at vmd_domain_reset(), it iterates bus from 0 to max_buses and
> calculates the MMIO address like this:
> 
> drivers/pci/controller/vmd.c:vmd_domain_reset() {
>     ...
>     for (bus = 0; bus < max_buses; bus++) {
>         for (dev = 0; dev < 32; dev++) {
>             base = vmd->cfgbar + PCIE_ECAM_OFFSET(bus,
>                                     PCI_DEVFN(dev, 0), 0);
>     ...
> }
Fair point, i need to change vmd_cfg_addr to avoid reading out of
bound. I will also udpate ECAM calculation.

> If the VMD buses start at a higher number (e.g., 128), could
> this hardcoded loop cause vmd_domain_reset() to miss the actual VMD
> bridges and perform MMIO reads/writes to incorrect offsets in the
> lower half of the BAR?
> 
> >  	if (offset + len >=
> > resource_size(&vmd->dev->resource[VMD_CFGBAR])) return NULL;  
> 
> [ ... ]
> 
> > @@ -661,6 +688,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)
> > +{
> > +	u64 phys1, phys2, bar4_2840;
> > +	void __iomem *bar4;
> > +	u32 base_id;
> > +	u8 base_bus;
> > +
> > +	bar4 = pci_ioremap_bar(vmd->dev, 4);
> > +	if (!bar4)
> > +		return -ENOMEM;
> > +
> > +	/* Read shadow registers for MEMBAR1 and MEMBAR2 physical
> > addresses. */
> > +	phys1 = readq(bar4 + SHADOW_MEMBAR1_28C1);
> > +	phys2 = readq(bar4 + SHADOW_MEMBAR2_28C1);  
> 
> [Severity: Medium]
> Could these reads access unmapped kernel memory if the device's BAR4
> is smaller than expected?
> 
> Since SHADOW_MEMBAR1_28C1 and BASE_ID_REG_28C1 access offsets like
> 0x2818 and 0x2840, should we verify that pci_resource_len(vmd->dev,
> 4) is large enough to cover these offsets before performing the
> readq() calls?

> 
> > +	/*
> > +	 * Read and set bus start number from Base ID register.
> > 24-bit Base ID
> > +	 * register is part of 64-bit shadowed reqid hide range
> > register and
> > +	 * holds segment, bus, device and function.
> > +	 */  
>