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

[sashiko-bot@kernel.org]

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?

>  	/*
>  	 * 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);
    ...
}

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.
> +	 */

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