Re: [PATCH v5 5/8] PCI: vmd: Replace hardcoded values with enum and defines

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Leak of dynamically allocated resource names on the vmd_probe error path.

Pre-existing issues:
- [High] Missing pci_free_irq_vectors() calls in error paths and during device removal, leading to vector leakage.
- [High] vmd_cleanup_srcu() destroys SRCU structs before devm-managed IRQ handlers are unregistered.
--

commit 415d4461cb0ec06aaac750c476d8975aa2ffae22
Author: Szymon Durawa <szymon.durawa@linux.intel.com>

PCI: vmd: Replace hardcoded values with enum and defines

Add enum vmd_resource type to replace hardcoded values. Add defines for
vmd bus start number based on VMD restriction value. No functional
changes.

> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index c0a953df5f238..9ff82a24e859c 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
[ ... ]
> @@ -794,7 +806,8 @@ static void vmd_configure_cfgbar(struct vmd_dev *vmd)
>   *
>   * Function fills resource buffer inside the VMD structure.
>   */
> -static void vmd_configure_membar(struct vmd_dev *vmd, u8 resource_number,
> +static void vmd_configure_membar(struct vmd_dev *vmd,
> +				 enum vmd_resource resource_number,
>  				 u8 membar_number, resource_size_t start_offset,
>  				 resource_size_t end_offset)
>  {

[Severity: Medium]
Does this memory allocation lack proper cleanup in error paths?

In vmd_configure_membar(), resource names are allocated dynamically:

    .name = kasprintf(GFP_KERNEL, "VMD MEMBAR%d %s", membar_number / 2, ...),

If vmd_enable_domain() fails after these names are successfully allocated,
the error propagates to vmd_probe(). The error handling block at the
out_release_instance label returns without explicitly freeing these allocated
strings, which leaks memory.

[ ... ]
> @@ -1104,8 +1119,8 @@ static void vmd_remove(struct pci_dev *dev)
>  	pci_remove_root_bus(vmd->bus);
>  
>  	/* CFGBAR is static, does not require releasing memory */
> -	kfree(vmd->resources[1].name);
> -	kfree(vmd->resources[2].name);
> +	kfree(vmd->resources[VMD_RES_MBAR_1].name);
> +	kfree(vmd->resources[VMD_RES_MBAR_2].name);

[Severity: High]
This isn't a bug introduced by this patch, but does this function omit the
required call to pci_free_irq_vectors()?

The PCI subsystem guidelines require that after a successful call to
pci_alloc_irq_vectors(), every error path and device removal must explicitly
call pci_free_irq_vectors() to prevent leaking IRQ vectors.

If init_srcu_struct() or devm_request_irq() fails in vmd_alloc_irqs(), or if
any operation fails in vmd_enable_domain() after allocation, the function
returns without cleanup. 

Similarly, this vmd_remove() function completes tear-down without freeing 
the vectors. Exhausting system IRQ capacity could prevent future allocations.

>  
>  	vmd_cleanup_srcu(vmd);

[Severity: High]
This isn't a bug introduced by this patch, but does cleaning up SRCU here
create a race condition with the devm-managed IRQ handler?

During device removal, vmd_remove() calls vmd_cleanup_srcu() to destroy
the SRCU structs. However, the driver's IRQ handlers were registered using
devm_request_irq(), which means they aren't deregistered until the devres
cleanup phase executes after vmd_remove() returns.

If a spurious MSI interrupt fires during this window, the handler will run
and invoke srcu_read_lock() on the already destroyed srcu_struct:

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

Could this use-after-free on per-CPU memory lead to memory corruption?

>  	vmd_detach_resources(vmd);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625082940.1290740-1-szymon.durawa@linux.intel.com?part=5