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 <[email protected]> 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/[email protected]?part=30
