Re: [RFC PATCH 6/8] PCI/IOV: Let sriov_add_vfs() own the failure unwind

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Signed integer overflow in pci_iov_virtfn_bus() and pci_iov_virtfn_devfn() when computing the routing ID for a virtual function.
- [High] Preexisting lockdep assertion failure when EEH driver removes a Virtual Function without holding the pci_rescan_remove_lock.
--

commit b6383e6825acdb6d59417c0f87bde1ecfbf1a2b6
Author: Pavol Sakac <sakacpav@amazon.de>

PCI/IOV: Let sriov_add_vfs() own the failure unwind

This commit modifies the PCI/IOV virtualization code to handle VF addition
failures gracefully without requiring the pci_rescan_remove_lock in async
workers. It pushes the failure unwind responsibility to the callers using
pci_iov_remove_virtfn().

> Leave __pci_iov_add_virtfn() reporting only and let each caller unwind
> through pci_iov_remove_virtfn(), whose lookup-based design is correct at
> every failure stage. sriov_add_vfs() unwinds ids 0..i inclusive on
> failure of VF i, since VF i may be registered but not yet linked. The
> wrapper unwinds fully before returning, because its EEH caller discards
> the return code: the VF is removed through pci_iov_remove_virtfn(),

[Severity: High]
This isn't a bug introduced by this patch, but does this EEH caller
invocation violate core kernel locking rules?

Looking at the EEH driver, it calls pci_iov_remove_virtfn() to tear down
a VF without holding pci_rescan_remove_lock:

arch/powerpc/kernel/eeh_driver.c:eeh_rmv_device() {
    ...
    if (edev->physfn) {
        ...
        pci_iov_remove_virtfn(edev->physfn, edev->vf_index);
        ...
    }
    ...
}

However, pci_iov_remove_virtfn() calls pci_stop_and_remove_bus_device(),
which explicitly requires pci_rescan_remove_lock to be held, as verified by
lockdep_assert_held(&pci_rescan_remove_lock). Removing a device without
this lock risks concurrent PCI hotplug operations, which could lead to race
conditions and use-after-free bugs during EEH error recovery.

> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index dda9303516f5d..a32b2c2959223 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
[ ... ]
> @@ -394,8 +397,9 @@ int pci_iov_add_virtfn(struct pci_dev *dev, int id)
>  {
>  	struct pci_bus *bus;
> +	bool created;
>  	int rc;
>  
> -	bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id), NULL);
> +	bus = virtfn_add_bus(dev->bus, pci_iov_virtfn_bus(dev, id), &created);

[Severity: High]
This is a pre-existing issue, but is there a signed integer overflow
vulnerability in the pci_iov_virtfn_bus() offset calculation?

Looking at pci_iov_virtfn_bus(), the calculation relies on multiplying
stride and vf_id:

drivers/pci/iov.c:pci_iov_virtfn_bus() {
    ...
    return dev->bus->number + ((dev->devfn + dev->sriov->offset +
                                dev->sriov->stride * vf_id) >> 8);
}

The dev->sriov->stride is a u16 and vf_id is an int. This evaluates to a
32-bit signed integer. If a malicious or misconfigured PCI device
advertises a large total_VFs and stride in its SR-IOV Extended Capability
structure, couldn't the multiplication overflow and produce a negative bus
offset?

This could underflow the bus number and cause the kernel to insert the new
bus with an out-of-bounds or corrupted identifier, resulting in memory
corruption in the PCI resource tree.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-vfopt-s1-v1-0-693271dc0226@amazon.de?part=6