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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] sfc: inverted error check for cxl_map() causes UAF on success
- [Medium] pci: reference leak of pf0_pci_dev in efx_cxl_init()
- [Medium] sfc: memory leak of non-PF0 cxl struct in efx_cxl_exit()
--

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

    sfc: add multipf support
    
    Use CXL core accelerator API for registering non-PF0 PFs to the memdev
    linked to the PF0, along with its complementary unregister.
    
    Adapt the ioremap call per PF to be an offset based on the PF function
    index and a hardcoded per PF CXL.mem slot size.

> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c
> index 348d7404cd7aa..bed8d9c591853 100644
> --- a/drivers/net/ethernet/sfc/efx_cxl.c
> +++ b/drivers/net/ethernet/sfc/efx_cxl.c
> @@ -21,8 +46,48 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
[ ... ]
> +	/* are we PF0? */
> +	devfn = PCI_FUNC(pci_dev->devfn);
> +	if (devfn != 0) {
> +		struct pci_dev *pf0_pci_dev;
> +		struct cxl_memdev *cxlmd;
> +
> +		pf0_pci_dev = pci_get_slot(pci_dev->bus,
> +					   PCI_DEVFN(PCI_SLOT(pci_dev->devfn),
> +						     0));

[Severity: Medium]
Does this code leak the reference to the parent PCI device structure?

pci_get_slot() increments the reference count of the returned PCI device.
However, pci_dev_put() is never called to release this reference in any of the
subsequent return paths within the non-PF0 branch of efx_cxl_init().

> +		/* 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;
[ ... ]
> +		cxl = kzalloc_obj(*cxl, GFP_KERNEL);
> +		if (!cxl)
> +			return -ENOMEM;

[Severity: Medium]
Does the non-PF0 cxl struct memory get leaked during device removal or probe
failure?

The cxl object is allocated manually here using kzalloc_obj(), whereas for PF0
devices it uses devm_cxl_dev_state_create() which is automatically freed via
device resource management.

Since the unified teardown function efx_cxl_exit() only unmaps the I/O memory
and doesn't call kfree(probe_data->cxl), wouldn't this manual allocation be
leaked?

> +
> +		cxl->cxlmd = cxlmd;
> +
> +		if (!cxl_map(probe_data, cxl, (u64)devfn, cxl_pio_range)) {
> +			kfree(cxl);
> +			return -ENOMEM;
> +		}

[Severity: High]
Is the error check for cxl_map() inverted here?

cxl_map() returns 0 on success and sets probe_data->cxl:

static int cxl_map(struct efx_probe_data *probe_data, struct efx_cxl *cxl, ...)
{
        ...
        probe_data->cxl = cxl;
        ...
        return 0;
}

Because of the inverted check, a success (0) evaluates to true, triggering
this error path which calls kfree(cxl) and returns -ENOMEM.

When the device probe eventually fails, the teardown path efx_cxl_exit() will
see that probe_data->cxl is non-NULL and attempt to dereference it to unmap
I/O memory:

void efx_cxl_exit(struct efx_probe_data *probe_data)
{
        if (!probe_data->cxl)
                return;

        iounmap(probe_data->cxl->ctpio_cxl);
}

Could this lead to a Use-After-Free crash on every successful mapping of a
non-PF0 device?

> +		return 0;
> +	}
[ ... ]
> @@ -80,16 +145,8 @@ int efx_cxl_init(struct efx_probe_data *probe_data)
>  		return PTR_ERR(cxl->cxlmd);
>  	}
>  
> -	cxl->ctpio_cxl = ioremap_wc(cxl_pio_range.start,
> -				    range_len(&cxl_pio_range));
> -	if (!cxl->ctpio_cxl) {
> -		pci_err(pci_dev, "CXL ioremap region (%pra) failed\n",
> -			&cxl_pio_range);
> +	if (!cxl_map(probe_data, cxl, 0, cxl_pio_range))
>  		return -ENOMEM;

[Severity: High]
Does this similarly abort initialization on success for the PF0 path due to the
inverted error check on cxl_map()?

> -	}
> -
> -	probe_data->cxl_pio_initialised = true;
> -	probe_data->cxl = cxl;
>  
>  	return 0;
>  }

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