Re: [RFC 04/12] PCI: Convert arch/powerpc to pci_is_sriov_* helpers

[sashiko-bot@kernel.org]

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

New issues:
- [High] Missed conversions of SR-IOV specific is_virtfn and is_physfn checks.

Pre-existing issues:
- [High] Unconditional NULL pointer dereference in pnv_pci_sriov_disable() if PE allocation fails.
- [High] Unchecked NULL pointer dereference in pnv_pci_ioda_fixup_iov() for VFs with failed configuration.
- [High] Failure to unshift IOV BAR resources in pnv_pci_sriov_disable() if the first VF's PE failed to configure.
- [High] Lockless list traversal of phb->ioda.pe_list exposes traversal to concurrent modifications.
--

commit 5e49eb3e31cb4ac469a301b263f891c4ba0713e3
Author: Dimitri Daskalakis <daskald@meta.com>

PCI: Convert arch/powerpc to pci_is_sriov_* helpers

This patch converts SR-IOV specific is_physfn and is_virtfn reads in the
PowerPC PCI code to use pci_is_sriov_physfn() and pci_is_sriov_virtfn().
This is intended to keep SR-IOV semantics intact once those flags are
widened to cover other virtualization types.

> Convert SR-IOV-specific is_physfn / is_virtfn reads in the PowerPC
> PCI code to use pci_is_sriov_physfn() / pci_is_sriov_virtfn(). These
> call sites are all SR-IOV-specific: they guard SR-IOV state
> dereferences, VF PE management, or sit inside #ifdef CONFIG_PCI_IOV
> blocks. 

[Severity: High]
The commit message states that all SR-IOV specific is_physfn and is_virtfn
reads are converted, but did we miss some checks in other PowerPC PCI files?

For instance, in arch/powerpc/kernel/eeh.c:eeh_break_device():

    if (pdev->is_virtfn) {
        ...
        pos = pci_find_ext_capability(pdev->physfn, PCI_EXT_CAP_ID_SRIOV);
        ...

If a non-SR-IOV VF (like SIOV) has is_virtfn set, wouldn't this read from
pdev->physfn (which might be NULL) or fail to find the SR-IOV capability
(pos = 0) and corrupt the PCI config space at offset 8 (PCI_SRIOV_CTRL)?

Similarly, there appear to be missed conversions guarding SR-IOV logic in:
- arch/powerpc/platforms/powernv/eeh-powernv.c:pnv_eeh_get_upstream_pe()
- arch/powerpc/kernel/eeh_sysfs.c:eeh_notify_resume_add()
- arch/powerpc/kernel/eeh_sysfs.c:eeh_notify_resume_remove()


> diff --git a/arch/powerpc/platforms/powernv/pci-sriov.c b/arch/powerpc/platforms/powernv/pci-sriov.c
> index 7105a573aec42..1113488f43720 100644
> --- a/arch/powerpc/platforms/powernv/pci-sriov.c
> +++ b/arch/powerpc/platforms/powernv/pci-sriov.c
> @@ -225,7 +225,7 @@ static void pnv_pci_ioda_fixup_iov_resources(struct pci_dev *pdev)
>  
>  void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
>  {
> -	if (pdev->is_virtfn) {
> +	if (pci_is_sriov_virtfn(pdev)) {
>  		struct pnv_ioda_pe *pe = pnv_ioda_get_pe(pdev);
>  
>  		/*
> @@ -235,7 +235,7 @@ void pnv_pci_ioda_fixup_iov(struct pci_dev *pdev)
>  		 */
>  		pe->pdev = pdev;

[Severity: High]
This is a pre-existing issue, but can pnv_ioda_get_pe() return NULL here,
leading to a NULL pointer dereference when assigning pe->pdev = pdev?

During SR-IOV enablement, if pnv_ioda_configure_pe() fails for a specific VF
in pnv_ioda_setup_vf_PE(), the error is ignored and the VF's PE number
remains IODA_INVALID_PE. When the PCI core subsequently adds the VF and calls
this function, pnv_ioda_get_pe() will return NULL because the PE number is
invalid.

[ ... ]

> @@ -479,7 +479,7 @@ static void pnv_ioda_release_vf_PE(struct pci_dev *pdev)
>  
>  	phb = pci_bus_to_pnvhb(pdev->bus);
>  
> -	if (!pdev->is_physfn)
> +	if (!pci_is_sriov_physfn(pdev))
>  		return;
>  
>  	/* FIXME: Use pnv_ioda_release_pe()? */
>  	list_for_each_entry_safe(pe, pe_n, &phb->ioda.pe_list, list) {

[Severity: High]
This is a pre-existing issue, but does this lockless traversal of
phb->ioda.pe_list expose us to concurrent modifications?

The lock phb->ioda.pe_list_mutex is only held briefly during list_del()
inside this loop. However, other PCI hotplug paths (such as
pnv_ioda_setup_PE()) add entries to this list while holding the mutex.

If a concurrent hotplug event mutates the list during this traversal,
could it corrupt the cached pe_n pointer or list linkages, leading to
a use-after-free?

[ ... ]

[Severity: High]
This is a pre-existing issue, but I noticed a potential NULL pointer
dereference in pnv_pci_sriov_disable() in the same file if PE allocation
fails:

arch/powerpc/platforms/powernv/pci-sriov.c:pnv_pci_sriov_disable() {
    ...
    base_pe = iov->vf_pe_arr[0].pe_number;
    ...
}

In pnv_pci_sriov_enable(), if pnv_ioda_alloc_pe() fails, it returns -EBUSY
and iov->vf_pe_arr remains NULL. The generic PCI core handles this failure
by calling pcibios_sriov_disable(), which invokes pnv_pci_sriov_disable().
This unconditionally accesses iov->vf_pe_arr[0], which would crash.

[Severity: High]
This is another pre-existing issue in pnv_pci_sriov_disable(). Does the
error teardown path fail to unshift IOV BAR resources if the first VF's
PE failed to configure?

arch/powerpc/platforms/powernv/pci-sriov.c:pnv_pci_sriov_disable() {
    ...
    base_pe = iov->vf_pe_arr[0].pe_number;
    ...
    pnv_pci_vf_resource_shift(pdev, -base_pe);
    ...
}

During SR-IOV enablement, pnv_pci_vf_resource_shift() shifts the IOV BARs
based on the allocated base PE number. If pnv_ioda_configure_pe() fails
for the first VF (VF 0), it zero-initializes the PE structure. During
error teardown, base_pe is read as 0, resulting in a call to
pnv_pci_vf_resource_shift(pdev, -0). This fails to unshift the IOV BARs
back to their original addresses, which could leak the shifted MMIO windows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604150153.3619662-1-dimitri.daskalakis1@gmail.com?part=4