Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Incomplete validation of `nr_recs` in `mock_clear_event` allows an out-of-bounds array access and arbitrary memory free. --
commit fb02931b1ff4b8cef05ff7386cba792c02a0c46e Author: Ira Weiny <[email protected]> tools/testing/cxl: Make event logs dynamic This commit modifies the mock event logs in the cxl test tool to be dynamically allocated instead of using static arrays. This is intended to support dynamically generating events when extents are created or destroyed. > diff --git a/tools/testing/cxl/test/mem.c b/tools/testing/cxl/test/mem.c > index 271c7ad8cc32c..a2bfd52db0762 100644 > --- a/tools/testing/cxl/test/mem.c > +++ b/tools/testing/cxl/test/mem.c [ ... ] > @@ -325,23 +354,20 @@ static int mock_clear_event(struct device *dev, struct > cxl_mbox_cmd *cmd) > if (!log) > return 0; /* No mock data in this log */ > > - /* > - * This check is technically not invalid per the specification AFAICS. > - * (The host could 'guess' handles and clear them in order). > - * However, this is not good behavior for the host so test it. > - */ > - if (log->clear_idx + pl->nr_recs > log->cur_idx) { > - dev_err(dev, > - "Attempting to clear more events than returned!\n"); > - return -EINVAL; > - } > + guard(write_lock)(&log->lock); > > /* Check handle order prior to clearing events */ > - for (nr = 0, handle = event_get_clear_handle(log); > - nr < pl->nr_recs; > - nr++, handle++) { > + handle = log->current_handle; > + for (nr = 0; nr < pl->nr_recs && handle != log->last_handle; > + nr++, handle = event_inc_handle(handle)) { > + > + dev_dbg(dev, "Checking clear of %d handle %u plhandle %u\n", > + log_type, handle, > + le16_to_cpu(pl->handles[nr])); > + > if (handle != le16_to_cpu(pl->handles[nr])) { > - dev_err(dev, "Clearing events out of order\n"); > + dev_err(dev, "Clearing events out of order %u %u\n", > + handle, le16_to_cpu(pl->handles[nr])); > return -EINVAL; > } > } > @@ -350,25 +376,12 @@ static int mock_clear_event(struct device *dev, struct > cxl_mbox_cmd *cmd) > log->nr_overflow = 0; > > /* Clear events */ > - log->clear_idx += pl->nr_recs; > - return 0; > -} > + for (nr = 0; nr < pl->nr_recs; nr++) > + mes_del_event(dev, log, le16_to_cpu(pl->handles[nr])); [Severity: Medium] Can this result in an out-of-bounds access and arbitrary memory free? If a user issues a clear command where pl->nr_recs is greater than the number of valid events, the validation loop above terminates early when handle == log->last_handle. It does not return an error for the extra records. The deletion loop here then unconditionally iterates pl->nr_recs times, passing the remaining, unvalidated user-provided handles to mes_del_event(). Inside mes_del_event(): static void mes_del_event(struct device *dev, struct mock_event_log *log, u16 handle) { ... record = log->events[handle]; ... devm_kfree(dev, record); } Since the user-provided handles are not validated against the log->events array size bounds when the validation loop terminates early, does this allow an out-of-bounds read on the array followed by freeing whatever value is read? > + dev_dbg(dev, "Delete log %d cur %d last %d\n", > + log_type, log->current_handle, log->last_handle); > > -static void cxl_mock_event_trigger(struct device *dev) > -{ -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=29
