Re: [PATCH v2 4/4] sfc: add multipf support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] sfc: inverted error check causes uninitialized memory mapping
- [High] sfc: incorrect slot extraction links non-PF0 to wrong PF0 device
--

commit f448ba0f4a9e4a95bf07bd91e1ee5f0a1a822aff
Author: Alejandro Lucero <alucerop@amd.com>

sfc: add multipf support

This commit introduces multipf support by using the CXL core accelerator API
to link non-PF0 PFs to the CXL region of their associated PF0. It obtains
the CXL HPA region and maps it using an offset based on the PF index.

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7aa..f884580cb5286 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
[ ... ]
> @@ -13,8 +13,69 @@
[ ... ]
> +static int efx_cxl_non_pf0_init(struct efx_probe_data *probe_data)
> +{
> +	struct efx_nic *efx = &probe_data->efx;
> +	struct pci_dev *pci_dev = efx->pci_dev;
> +	struct range cxl_pio_range;
> +	struct efx_cxl *cxl;
> +	u64 devfn;
> +
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +
> +	struct pci_dev *pf0_pci_dev __free(pci_dev_put) =
> +		pci_get_slot(pci_dev->bus, PCI_DEVFN(PCI_SLOT(devfn), 0));

[Severity: High]
Does this code query the correct PCI slot? The devfn variable is assigned the
result of PCI_FUNC(pci_dev->devfn), which extracts only the 3-bit function
number. Passing this isolated function number into PCI_SLOT() will always
yield 0 since it shifts right by 3.

This means non-PF0 functions will query slot 0 instead of their actual PCI
slot. If the NIC is located in a slot other than 0 and another CXL-enabled
NIC is in slot 0, would this cause the driver to link its non-PF0 function
to the wrong NIC's CXL region, bypassing mutual exclusion by taking the wrong
device's lock in cxl_get_range_and_link()?

> +
> +	/* This should not happen! */
> +	if (!pf0_pci_dev)
> +		return 0;
> +
> +	/* Is the PF0 device configured with and using CXL? */
> +	if (!pcie_is_cxl(pf0_pci_dev))
> +		return 0;
> +
> +	if (!cxl_get_range_and_link(&pf0_pci_dev->dev, &pci_dev->dev,
> +				    &cxl_pio_range))
> +		return  -EPROBE_DEFER;

[Severity: High]
Is the error check here inverted? If cxl_get_range_and_link() successfully
finds a region (returning 0), this condition evaluates to true and aborts
with -EPROBE_DEFER.

Conversely, if it fails (e.g., due to a missing region), it returns an error
code which evaluates as true, meaning the condition fails and the code falls
through. This leaves the cxl_pio_range stack variable uninitialized.

> +
> +	cxl = kzalloc_obj(*cxl, GFP_KERNEL);
> +	if (!cxl)
> +		return -ENOMEM;
> +
> +	if (cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {

[Severity: High]
Following up on the inverted check above, if cxl_get_range_and_link() fails
and we fall through, won't we pass the uninitialized cxl_pio_range stack
variable by value to cxl_map()?

Looking at cxl_map() in this same file:

cxl_map(...) {
    ...
    cxl_pio_pf_start = cxl_pio_range.start + ...;
    cxl->ctpio_cxl = ioremap_wc(cxl_pio_pf_start, ...);
    ...
}

Could this result in ioremap_wc() mapping arbitrary physical memory based
on the uninitialized stack data?

> +		kfree(cxl);
> +		return -ENOMEM;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001132023.17032-1-alucerop@amd.com?part=4