Re: [PATCH v11 30/31] tools/testing/cxl: Add DC Regions to mock mem data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Medium] Missing bounds validation on `cmd->size_out` causes out-of-bounds writes in `mock_get_dc_extent_list`.
- [Medium] `mock_get_dc_extent_list` fails to update `cmd->size_out` to the actual response size.
- [Medium] Unconditionally clearing sent extents in `mock_add_dc_response` breaks the host's ability to issue multi-part responses.
- [Medium] Missing bounds validation on `cmd->size_in` causes out-of-bounds reads in mailbox commands.
- [Medium] Missing `devm_kfree()` calls cause unbounded memory accumulation of devres structures when extents are removed.
- [Medium] Validation failures mid-command leave the mock device in a torn state due to partial state mutation.
- [Medium] Flawed overlap validation in `new_extent_valid()` allows injection of overlapping extents.
- [Medium] Concurrent sysfs operations create a race condition resulting in orphaned mock events.
--

commit 8e9b7fc74b6af2c32bcba55480acc53f67c13c08
Author: Ira Weiny <iweiny@kernel.org>

tools/testing/cxl: Add DC Regions to mock mem data

This commit enhances cxl_test by introducing mock memory data for Dynamic
Capacity (DC) regions. It provides simulated sysfs interfaces to inject and
delete extents, alongside emulated mailbox commands to interact with them.

> diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c
> index a2bfd52db0762..455f8a50d5818 100644
> --- a/tools/testing/cxl/test/mem.c
> +++ b/tools/testing/cxl/test/mem.c
[ ... ]
> @@ -1594,6 +1861,215 @@ static int mock_get_supported_features(struct cxl_mockmem_data *mdata,
>  	return 0;
>  }
>  
> +static int mock_get_dc_config(struct device *dev,
[ ... ]
> +static int mock_get_dc_extent_list(struct device *dev,
> +				   struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mbox_get_extent_out *resp = cmd->payload_out;
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	struct cxl_mbox_get_extent_in *get = cmd->payload_in;
> +	u32 total_avail = 0, total_ret = 0, idx = 0;
> +	struct cxl_extent_data *ext;
> +	u32 ext_count, start_idx;
> +	unsigned long i;
> +
> +	ext_count = le32_to_cpu(get->extent_cnt);
> +	start_idx = le32_to_cpu(get->start_extent_index);
> +
> +	memset(resp, 0, sizeof(*resp));
> +
> +	guard(mutex)(&mdata->ext_lock);

[Severity: Medium]
Does `cmd->size_out` need to be validated against the expected output size
before we `memset` and write into the flexible array? If the caller provides
a buffer smaller than `sizeof(*resp) + total_ret * sizeof(struct cxl_extent)`,
could this overflow the output buffer?

> +	/*
> +	 * Total available needs to be calculated and returned regardless of
> +	 * how many can actually be returned.
> +	 */
> +	xa_for_each(&mdata->dc_accepted_exts, i, ext)
> +		total_avail++;
> +
> +	if (start_idx > total_avail)
> +		return -EINVAL;
> +
> +	xa_for_each(&mdata->dc_accepted_exts, i, ext) {
> +		if (total_ret >= ext_count)
> +			break;
> +
> +		if (idx++ >= start_idx) {
> +			resp->extent[total_ret].start_dpa =
> +						cpu_to_le64(ext->dpa_start);
> +			resp->extent[total_ret].length =
> +						cpu_to_le64(ext->length);
> +			export_uuid(resp->extent[total_ret].uuid, &ext->uuid);
> +			resp->extent[total_ret].shared_extn_seq =
> +						cpu_to_le16(ext->shared_extn_seq);
> +			total_ret++;
> +		}
> +	}
> +
> +	resp->returned_extent_count = cpu_to_le32(total_ret);
> +	resp->total_extent_count = cpu_to_le32(total_avail);
> +	resp->generation_num = cpu_to_le32(mdata->dc_ext_generation);
> +
> +	dev_dbg(dev, "Returning %d extents of %d total\n",
> +		total_ret, total_avail);
> +
> +	return 0;
> +}

[Severity: Medium]
Is it expected that `cmd->size_out` is left unmodified on success here? Other
mock mailbox commands update this field to reflect the actual response payload
size (e.g., setting it to `struct_size(resp, extent, total_ret)`).

> +
> +static void dc_clear_sent(struct device *dev)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	struct cxl_extent_data *ext;
> +	unsigned long index;
> +
> +	lockdep_assert_held(&mdata->ext_lock);
> +
> +	/* Any extents not accepted must be cleared */
> +	xa_for_each(&mdata->dc_sent_extents, index, ext) {
> +		dev_dbg(dev, "Host rejected extent %#llx\n", ext->dpa_start);
> +		xa_erase(&mdata->dc_sent_extents, ext->dpa_start);
> +	}
> +}

[Severity: Medium]
Does removing the extent from the XArray via `xa_erase()` leak the underlying
`cxl_extent_data` structure? Since it was originally allocated with
`devm_kzalloc()`, it appears this memory will accumulate indefinitely over
repeated insert/erase cycles unless `devm_kfree()` is explicitly called.
This identical pattern exists in `dc_delete_extent()` and
`release_accepted_extent()` as well.

> +
> +static int mock_add_dc_response(struct device *dev,
> +				struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mbox_dc_response *req = cmd->payload_in;
> +	u32 list_size = le32_to_cpu(req->extent_list_size);
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	u32 last_offer_seq = 0;
> +	bool first = true;
> +
> +	guard(mutex)(&mdata->ext_lock);
> +	for (int i = 0; i < list_size; i++) {
> +		u64 start = le64_to_cpu(req->extent_list[i].dpa_start);
> +		u64 length = le64_to_cpu(req->extent_list[i].length);

[Severity: Medium]
Is it safe to iterate up to `list_size` without first validating that
`cmd->size_in` is large enough to contain
`struct_size(req, extent_list, list_size)`? Could a malformed request read
out-of-bounds memory here?

> +		struct cxl_extent_data *ext;
> +		int rc;
> +
> +		/*
> +		 * CXL r4.0 8.2.10.9.9.3: the host must list extents in the
> +		 * order the device offered them (Add Capacity events); reject
> +		 * an out-of-order response as Invalid Input.
> +		 */
> +		ext = xa_load(&mdata->dc_sent_extents, start);
> +		if (!ext)
> +			ext = xa_load(&mdata->dc_accepted_exts, start);
> +		if (ext) {
> +			if (!first && ext->offer_seq < last_offer_seq) {
> +				dev_err(dev, "Add-DC-Response out of order at extent %#llx\n",
> +					start);
> +				return -EINVAL;
> +			}
> +			last_offer_seq = ext->offer_seq;
> +			first = false;
> +		}
> +
> +		rc = dc_accept_extent(dev, start, length);
> +		if (rc)
> +			return rc;
> +	}

[Severity: Medium]
Could returning an error midway through this loop leave the mock device in a
torn state? If an extent midway through the list fails validation (like the
out-of-order check), previous extents in the iteration have already been
irrevocably transitioned to the accepted state via `dc_accept_extent()`.

> +
> +	dc_clear_sent(dev);
> +	return 0;
> +}

[Severity: Medium]
Will unconditionally calling `dc_clear_sent(dev)` break host responses that are
split into multiple parts? If the host sets `req->flags & CXL_DCD_EVENT_MORE`,
this clears the remaining pending extents before the host can acknowledge them.

> +
> +static void dc_delete_extent(struct device *dev, unsigned long long start,
> +			     unsigned long long length)
> +{
[ ... ]
> +static int mock_dc_release(struct device *dev,
> +			   struct cxl_mbox_cmd *cmd)
> +{
> +	struct cxl_mbox_dc_response *req = cmd->payload_in;
> +	u32 list_size = le32_to_cpu(req->extent_list_size);
> +
> +	for (int i = 0; i < list_size; i++) {
> +		u64 start = le64_to_cpu(req->extent_list[i].dpa_start);
> +		u64 length = le64_to_cpu(req->extent_list[i].length);

[Severity: Medium]
Similarly to `mock_add_dc_response()`, does `mock_dc_release()` need to
validate that `cmd->size_in` is large enough to hold `list_size` entries before
iterating over `req->extent_list[i]`?

> +
> +		dev_dbg(dev, "Extent %#llx released by host\n", start);
> +		release_accepted_extent(dev, start, length);
> +	}
> +
> +	return 0;
> +}
[ ... ]
> @@ -1936,11 +2442,328 @@ static ssize_t sanitize_timeout_store(struct device *dev,
>  
>  static DEVICE_ATTR_RW(sanitize_timeout);
>  
> +/* Return if the proposed extent would break the test code */
> +static bool new_extent_valid(struct device *dev, size_t new_start,
> +			     size_t new_len)
> +{
> +	struct cxl_mockmem_data *mdata = dev_get_drvdata(dev);
> +	struct cxl_extent_data *extent;
> +	size_t new_end, i;
> +
> +	if (!new_len)
> +		return false;
> +
> +	new_end = new_start + new_len;
> +
> +	dev_dbg(dev, "New extent %zx-%zx\n", new_start, new_end);
> +
> +	guard(mutex)(&mdata->ext_lock);
> +	dev_dbg(dev, "Checking extents starts...\n");
> +	xa_for_each(&mdata->dc_fm_extents, i, extent) {
> +		if (extent->dpa_start == new_start)
> +			return false;
> +	}

[Severity: Medium]
Is it intentional that the overlap validation only checks exact starting
addresses? The iteration calculates `new_end` but doesn't check
`extent->length`, which seems to permit injecting extents that partially overlap
in physical DPA ranges.

> +
> +	dev_dbg(dev, "Checking sent extents starts...\n");
> +	xa_for_each(&mdata->dc_sent_extents, i, extent) {
> +		if (extent->dpa_start == new_start)
> +			return false;
> +	}
> +
> +	dev_dbg(dev, "Checking accepted extents starts...\n");
> +	xa_for_each(&mdata->dc_accepted_exts, i, extent) {
> +		if (extent->dpa_start == new_start)
> +			return false;
> +	}
> +
> +	return true;
> +}
[ ... ]
> +static ssize_t __dc_inject_extent_store(struct device *dev,
> +					struct device_attribute *attr,
> +					const char *buf, size_t count,
> +					bool shared)
> +{
[ ... ]
> +	if (!new_extent_valid(dev, start, length))
> +		return -EINVAL;
> +
> +	rc = devm_add_fm_extent(dev, start, length, uuid_str, shared_extn_seq,
> +				shared);
> +	if (rc) {
> +		dev_err(dev, "Failed to add extent DPA:%#llx LEN:%#llx; %d\n",
> +			start, length, rc);
> +		return rc;
> +	}
> +
> +	mark_extent_sent(dev, start);
> +	rc = log_dc_event(mdata, DCD_ADD_CAPACITY, start, length, uuid_str,
> +			  shared_extn_seq, more);
> +	if (rc) {

[Severity: Medium]
Could concurrent sysfs operations result in orphaned events here?
`mark_extent_sent()` aborts silently if the extent was deleted concurrently
(e.g., from a sysfs delete executing at the same time), but
`__dc_inject_extent_store()` will still unconditionally queue a
`DCD_ADD_CAPACITY` event via `log_dc_event()`.

> +		dev_err(dev, "Failed to add event %d\n", rc);
> +		return rc;
> +	}
> +
> +	return count;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625112638.550691-1-anisa.su@samsung.com?part=30