Re: [PATCH v1 3/4] cxl/memdev: Add support for multi PF devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] cxl: UAF / memory corruption due to omitted no_free_ptr() in cxl_get_pf0_memdev()
- [High] cxl: NULL pointer dereference when cxlmd->attach is NULL
- [High] cxl: data race and NULL pointer dereference due to missing memory barriers
--

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

cxl/memdev: Add support for multi PF devices

This commit adds a device link between the CXL region a PF0 memdev is
attached to and the non-PF0 wanting to use the CXL region. A CXL region
release will trigger such a PF to be released from its driver first,
keeping the CXL memory use safe.

> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index b3419df586b9c..67be02faa7e14 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
[ ... ]
> @@ -802,6 +802,72 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_dev_state *cxlds,
> +struct cxl_memdev *cxl_get_pf0_memdev(struct device *pf0, struct device *pfx,
> +				      struct range *range)
> +{
> +	struct cxl_attach_region *attach;
> +	struct cxl_memdev *cxlmd;
> +	struct device *mem_dev __free(put_device) =
> +		bus_find_device(&cxl_bus_type, NULL, pf0,
> +				match_memdev_by_parent_device);
> +
> +	if (!mem_dev)
> +		return ERR_PTR(-ENODEV);
> +
> +	cxlmd = to_cxl_memdev(mem_dev);
> +
> +	/*
> +	 * we got the cxl_memdev and the implicit get_device in bus_find_device
> +	 * makes the next steps safe.
> +	 */
> +	attach = container_of(cxlmd->attach, struct cxl_attach_region, attach);

[Severity: High]
If cxlmd->attach is NULL (e.g. for a standard class memory device initialized
via devm_cxl_add_memdev), does container_of() evaluate to NULL here since the
member is at offset 0?

> +
> +	/*
> +	 * The cxlmd object does exist and it can be found in the cxl bus after
> +	 * creation but before attach probe setting the proper HPA range. If so,
> +	 * the caller will need to try later.
> +	 */
> +	if (attach->hpa_range.end == -1)

[Severity: High]
Could this lead to a NULL pointer dereference when accessing
attach->hpa_range.end if attach evaluated to NULL above?

Additionally, since attach->hpa_range.end is checked locklessly to determine
if initialization is complete, could CPU or compiler reordering cause the
subsequent read of attach->cxlr to see an uninitialized or NULL value?

> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	/*
> +	 * Create the device link between the region and the consumer device.
> +	 * AUTOREMOVE_CONSUMER means the link implicitly to be removed if the
> +	 * consumer unbinds first with no consequences for the supplier.
> +	 */
> +	if (!device_link_add(pfx, &attach->cxlr->dev, DL_FLAG_AUTOREMOVE_CONSUMER))

[Severity: High]
If attach->cxlr is read as NULL due to missing memory barriers or acquire
semantics, could this dereference lead to a kernel panic?

> +		return ERR_PTR(-ENODEV);
> +
> +	range->start = attach->hpa_range.start;
> +	range->end = attach->hpa_range.end;
> +
> +	return to_cxl_memdev(mem_dev);

[Severity: High]
Will the device reference acquired by bus_find_device() be unconditionally
dropped here at scope exit due to the __free(put_device) cleanup attribute?

Should this return to_cxl_memdev(no_free_ptr(mem_dev)) to inhibit the
cleanup and safely transfer ownership of the acquired reference to the caller?

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