Re: [PATCH v5 4/5] vfio/spapr_tce: Normalize EEH IOA error injection addresses

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] vfio/spapr_tce: vfio_spapr_ioctl_eeh_pe_op() breaks non-BAR error injection
- [Medium] vfio/spapr_tce: vfio_spapr_eeh_normalize_addr() matches unset PCI BARs
--

commit 02ecc4e02395ee3674eee2a40d451772c566a3ad
Author: Narayana Murty N <nnmlinux@linux.ibm.com>

vfio/spapr_tce: Normalize EEH IOA error injection addresses

This patch normalizes IOA/MMIO error-injection addresses in the sPAPR
VFIO EEH ioctl path before dispatching to eeh_pe_inject_err(), translating
host Linux BAR resource addresses to PCI bus addresses.

> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 1c0eec228cc6f..dad9ba54bae60 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -774,6 +774,97 @@ static long tce_iommu_create_default_window(struct tce_container *container)
>  	return ret;
>  }
>  
> +static int vfio_spapr_eeh_normalize_addr(struct eeh_pe *pe,
> +					 unsigned long addr,
> +					 unsigned long mask,
> +					 unsigned long *normalized)
> +{

[ ... ]

> +	pci_lock_rescan_remove();
> +	eeh_pe_for_each_dev(pe, edev, tmp) {
> +		pdev = eeh_dev_to_pci_dev(edev);
> +		if (!pdev)
> +			continue;
> +
> +		for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +			res = &pdev->resource[bar];
> +
> +			if (!resource_size(res))
> +				continue;
> +
> +			if (!(res->flags & (IORESOURCE_MEM | IORESOURCE_IO)))
> +				continue;
> +
> +			pcibios_resource_to_bus(pdev->bus, &region, res);

[Severity: Medium]
Can vfio_spapr_eeh_normalize_addr() match unassigned or disabled BARs here?

On PowerPC, unassigned BARs in pcibios_fixup_resources() and
alloc_resource() in arch/powerpc/kernel/pci-common.c have res->start = 0,
res->end = size - 1, and IORESOURCE_UNSET set alongside IORESOURCE_MEM or
IORESOURCE_IO:

arch/powerpc/kernel/pci-common.c:pcibios_fixup_resources() {
    ...
			res->end -= res->start;
			res->start = 0;
			res->flags |= IORESOURCE_UNSET;
    ...
}

Because resource_size(res) is non-zero and IORESOURCE_UNSET is not checked,
vfio_spapr_eeh_normalize_addr() passes res to pcibios_resource_to_bus().
In drivers/pci/host-bridge.c, pcibios_resource_to_bus() calls
resource_contains(window->res, res), which returns false when
IORESOURCE_UNSET is set, leaving offset = 0 and setting region.start =
res->start.

When a device in the PE has an unassigned standard BAR, wouldn't any
supplied addr in [0, size) match Case 1 on the unassigned BAR, or if an
unassigned BAR retained res->start = window->offset from
of_pci_parse_addrs(), cause a valid CPU resource address of a later BAR at
the start of the PHB window to match Case 1 on the unset BAR and skip
translation?

For comparison, eeh_break_device() in arch/powerpc/kernel/eeh.c skips
such resources before matching BARs. Should this loop also skip BARs
where !res->start or res->flags & (IORESOURCE_UNSET | IORESOURCE_DISABLED)?

> +
> +			pci_start = region.start;
> +			pci_len = resource_size(res);

[ ... ]

> +done:
> +	pci_unlock_rescan_remove();
> +
> +	if (rc)
> +		pr_debug("vfio EEH: addr=0x%lx not found in any PCI bus BAR of PE, injection may fail\n",
> +			 addr);
> +	return rc;
> +}
> +
>  static long vfio_spapr_ioctl_eeh_pe_op(struct iommu_group *group,
>  				       unsigned long arg)
>  {
> @@ -818,6 +909,20 @@ static long vfio_spapr_ioctl_eeh_pe_op(struct iommu_group *group,
>  		if (copy_from_user(&op, (void __user *)arg, minsz))
>  			return -EFAULT;
>  
> +		if (op.err.type == EEH_ERR_TYPE_32 ||
> +		    op.err.type == EEH_ERR_TYPE_64) {
> +			unsigned long normalized;
> +			int ret;
> +
> +			ret = vfio_spapr_eeh_normalize_addr(pe, op.err.addr,
> +							    op.err.mask,
> +							    &normalized);
> +			if (ret)
> +				return ret;
> +
> +			op.err.addr = normalized;
> +		}
> +
>  		return eeh_pe_inject_err(pe, op.err.type, op.err.func,
>  					 op.err.addr, op.err.mask);
>  	default:

[Severity: Medium]
Does calling vfio_spapr_eeh_normalize_addr() for all EEH_ERR_TYPE_32 and
EEH_ERR_TYPE_64 requests break non-BAR error injections such as DMA and
PCI config-space errors?

In arch/powerpc/include/uapi/asm/eeh.h, EEH_ERR_TYPE_32 and EEH_ERR_TYPE_64
are the only two error types and are used across all op.err.func values
(EEH_ERR_FUNC_MIN through EEH_ERR_FUNC_MAX), including PCI config-space
error functions (EEH_ERR_FUNC_LD_CFG_ADDR, EEH_ERR_FUNC_LD_CFG_DATA,
EEH_ERR_FUNC_ST_CFG_ADDR, EEH_ERR_FUNC_ST_CFG_DATA) and DMA error functions
(EEH_ERR_FUNC_DMA_RD_* and EEH_ERR_FUNC_DMA_WR_*).

When userspace invokes VFIO_EEH_PE_INJECT_ERR for a DMA or config-space
error function with a non-zero op.err.addr (such as an IOVA in the TCE
window or a config offset) or a non-zero op.err.mask,
vfio_spapr_eeh_normalize_addr() searches standard MEM/IO BARs, fails to
find a match, and returns -EINVAL, causing vfio_spapr_ioctl_eeh_pe_op() to
abort before calling eeh_pe_inject_err() (or corrupts op.err.addr if the
DMA address happens to fall within a host BAR range).

Also, notice that the pr_debug() in vfio_spapr_eeh_normalize_addr() says
"injection may fail", whereas vfio_spapr_ioctl_eeh_pe_op() immediately
returns ret (-EINVAL) without attempting injection. Should BAR address
normalization be restricted to MMIO/PIO op.err.func values, or pass the
address through when it is not a BAR-targeted function?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019101813.45085-1-nnmlinux@linux.ibm.com?part=4