Re: [PATCH v11 29/31] tools/testing/cxl: Make event logs dynamic

[sashiko-bot@kernel.org]

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 <iweiny@kernel.org>

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/20260625112638.550691-1-anisa.su@samsung.com?part=29