Re: [PATCH] PCI: vmd: Only assign _OSC features in bare metal OS.

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect logical condition for bare-metal identification copies _OSC flags in VMs.

Pre-existing issues:
- [High] Missing `pci_free_irq_vectors()` causes permanent IRQ resource leak.
- [High] Use-After-Free of SRCU structures on device teardown.
--

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

PCI: vmd: Only assign _OSC features in bare metal OS.

Copy valid _OSC flags from root bridge only when OS can provide correct
values. In virtual machine, all of the control features are disabled as
hypervisor will not passthrough root bridge information to VM.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index d4ae250d4bc6d..f9e38f2f926a7 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -910,8 +910,10 @@ static int vmd_enable_domain(struct vmd_dev *vmd, unsigned long features)
>  		return -ENODEV;
>  	}
>  
> -	vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> -				   to_pci_host_bridge(vmd->bus->bridge));
> +	/* Don't copy _OSC control flags in VM, it disables features.*/
> +	if (!offset[0] || !offset[1])

[Severity: High]
Does this logical condition correctly identify a bare-metal environment?

If a VM exposes only one non-zero MEMBAR offset, this evaluates to true,
which misidentifies the VM as bare metal. This results in copying the
host bridge flags and incorrectly disabling control features in the VM.

Should this use && instead of || to ensure both offsets are zero?

> +		vmd_copy_host_bridge_flags(pci_find_host_bridge(vmd->dev->bus),
> +					 to_pci_host_bridge(vmd->bus->bridge));
>  
>  	vmd_attach_resources(vmd);
>  	if (vmd->irq_domain)

[ ... ]

[Severity: High]
This is a pre-existing issue, but does vmd_remove() leak the IRQ vectors?

vmd_alloc_irqs() allocates vectors without a devm wrapper:

drivers/pci/controller/vmd.c:vmd_alloc_irqs() {
...
    vmd->msix_count = pci_alloc_irq_vectors(dev, vmd->first_vec + 1,
                                            vmd->msix_count, PCI_IRQ_MSIX);
...
}

However, the teardown function lacks a corresponding cleanup:

drivers/pci/controller/vmd.c:vmd_remove() {
...
    vmd_cleanup_srcu(vmd);
    vmd_detach_resources(vmd);
    vmd_remove_irq_domain(vmd);
...
}

Since pcim_enable_device() doesn't exempt explicit cleanup for this API,
will this cause a permanent IRQ resource leak on module unload?

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk with the
SRCU structures during device teardown?

In vmd_remove(), the driver manually cleans up SRCU primitives:

drivers/pci/controller/vmd.c:vmd_remove() {
...
    vmd_cleanup_srcu(vmd);
...
}

Because the device's IRQ handlers are managed by devm_request_irq(), they
are not freed or unmasked until after vmd_remove() returns.

If an MSI-X interrupt fires in the intermediate window, could vmd_irq()
access the destroyed srcu_struct?

drivers/pci/controller/vmd.c:vmd_irq() {
...
    idx = srcu_read_lock(&irqs->srcu);
...
}

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