Re: [PATCH v2 05/11] drm/xe/cper: Prepare Intel CPER error info from info queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Integer overflow in the info queue chunk bounds check allows a heap buffer overflow.
- [High] Missing bounds check on `chunk_size` against maximum chunk array length leads to a kernel stack out-of-bounds read and information leak.
- [High] Unbounded processing of info queue chunks can cause an infinite loop and Denial of Service.
- [Medium] Firmware-provided `total_size` is trusted over actual fetched bytes, leading to uninitialized memory being parsed as valid error logs.
- [Medium] Capping `num_logs` to `XE_RAS_NUM_COUNTERS` incorrectly truncates log streams and desynchronizes aggregate parsing.
- [Medium] The aggregate error counter parsing loop creates a sparse array when `num_logs == 0`, leading to the emission of corrupted empty CPER records.
--

commit 426ce853adfa4b210c0c07504ac804ae25dd5a2b
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: Prepare Intel CPER error info from info queue

This commit adds a function to assemble raw info queue data embedded in a
GET_COUNTER response and subsequent chunks into an Intel CPER error info
structure. The data is retrieved in chunks from the firmware and parsed
into detail or aggregate error records.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index e913235e9cceb..27c78800b5d27 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> +static struct xe_cper_einfo_entry *
> +prepare_cper_error_info(struct xe_device *xe,
> +			const struct xe_ras_get_counter_response *counter_resp,
> +			const struct xe_ras_error_class *error_class,
> +			u32 *count_out)
> +{
[ ... ]
> +	raw_buf = kzalloc(XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE, GFP_KERNEL);
> +	if (!raw_buf)
> +		return NULL;
> +
> +	/* Copy first chunk already embedded in the counter response */
> +	if (first_qhdr->chunk_size &&
> +	    first_qhdr->chunk_offset + first_qhdr->chunk_size <=
> +	    XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE) {

[Severity: Critical]
Is `chunk_offset + chunk_size` vulnerable to a 32-bit integer overflow here?
If the firmware provides large values, such as `chunk_offset = 0xFFFFFF00`
and `chunk_size = 0x200`, the sum would wrap around to 0x100, which is less
than `XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE` (5120). This would bypass the check
and allow the subsequent `memcpy` to write out-of-bounds on the heap.

> +		memcpy(raw_buf + first_qhdr->chunk_offset,
> +		       counter_resp->info_queue.queue_data,
> +		       first_qhdr->chunk_size);

[Severity: High]
Is `first_qhdr->chunk_size` validated against the actual capacity of the
source `queue_data` buffer? The check above only ensures it fits within
`XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE` (5120), but `queue_data` has a maximum
size of `XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE`. Since `counter_resp` is allocated
on the caller's stack, could a malicious or broken firmware return a
`chunk_size` greater than the array size and cause this `memcpy` to read past
the buffer, leaking adjacent kernel stack memory?

> +		iq_offset = first_qhdr->chunk_size;
> +	}
> +
> +	/* Fetch any remaining chunks */
> +	if (first_qhdr->flags & XE_RAS_INFO_QUEUE_FLAG_MORE_DATA) {
> +		iq_req.source_command			= XE_SYSCTRL_CMD_GET_COUNTER;
> +		iq_req.source_context			= counter_resp->counter;
> +		iq_req.queue_request.requested_size	= XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE;
> +		iq_req.queue_request.session_id		= counter_resp->counter;
> +
> +		do {
> +			struct xe_ras_info_queue_header *qhdr;
> +			u32 end;
> +
> +			iq_req.queue_request.requested_offset = iq_offset;
> +
> +			if (get_info_queue_data(xe, &iq_req, &iq_response))
> +				break;
> +
> +			qhdr = &iq_response.queue_response.queue_header;
> +			end  = qhdr->chunk_offset + qhdr->chunk_size;
> +
> +			if (end > XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE) {

[Severity: Critical]
Does the `qhdr->chunk_offset + qhdr->chunk_size` calculation here have the
same 32-bit integer overflow risk as the first chunk check, potentially
allowing out-of-bounds writes into `raw_buf`?

> +				xe_warn(xe, "[RAS]: CPER: info queue chunk out of bounds (offset=%u size=%u)\n",
> +					qhdr->chunk_offset, qhdr->chunk_size);
> +				break;
> +			}
> +
> +			memcpy(raw_buf + qhdr->chunk_offset,
> +			       iq_response.queue_response.queue_data,
> +			       qhdr->chunk_size);

[Severity: High]
Similar to the first chunk copy, is there a missing bounds check here to
ensure `qhdr->chunk_size` does not exceed the size of the stack-allocated
`iq_response.queue_response.queue_data` array, which could lead to another
stack out-of-bounds read?

> +
> +			if (!qhdr->chunk_size)
> +				break;
> +
> +			iq_offset += qhdr->chunk_size;
> +		} while (iq_response.queue_response.queue_header.flags &
> +			 XE_RAS_INFO_QUEUE_FLAG_MORE_DATA);

[Severity: High]
Does this `do-while` loop have a bounded maximum number of iterations or a
timeout? If a broken or malicious firmware repeatedly returns a valid small
chunk with the `XE_RAS_INFO_QUEUE_FLAG_MORE_DATA` flag set, could this
loop run indefinitely and cause a kernel soft lockup?

> +	}
> +
> +	raw_total = first_qhdr->total_size
> +		  ? min(first_qhdr->total_size, XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE)
> +		  : iq_offset;

[Severity: Medium]
If `get_info_queue_data()` fails or the fetch loop terminates early, `raw_buf`
is only partially populated. By unconditionally using `first_qhdr->total_size`
here (when non-zero) instead of `iq_offset`, won't the code later parse
trailing uninitialized (zero-filled) sections of `raw_buf` as valid dynamic
headers and error logs?

[ ... ]
> +	} else {
> +		/* Aggregate case: one einfo per dynamic-counter header */
> +		const struct xe_ras_info_queue_dynamic_counter_hdr *hdrs =
> +			(const struct xe_ras_info_queue_dynamic_counter_hdr *)raw_buf;
> +		const struct xe_ras_error_log *all_logs =
> +			(const struct xe_ras_error_log *)(raw_buf + headers_size);
> +		u32 avail_logs = (raw_total - headers_size) / sizeof(struct xe_ras_error_log);
> +		u32 log_offset = 0;
> +
> +		einfo_arr = kzalloc_objs(*einfo_arr, num_headers, GFP_KERNEL);
> +		if (!einfo_arr) {
> +			kfree(raw_buf);
> +			return NULL;
> +		}
> +
> +		for (i = 0; i < num_headers; i++) {
> +			u32 num_logs = min_t(u32, hdrs[i].counter, XE_RAS_NUM_COUNTERS);

[Severity: Medium]
Does capping `num_logs` to `XE_RAS_NUM_COUNTERS` (16) cause desynchronization
when parsing the raw log stream? If the hardware actually embedded more than
16 logs for a specific header, limiting `num_logs` to 16 means `log_offset`
will only advance by 16. Won't the remaining logs then be incorrectly parsed
as belonging to the next aggregate header in the stream?

> +			struct xe_cper_sec_intel_error_info *einfo;
> +			struct xe_intel_priv_event_entry *entry;
> +			u32 einfo_size;
> +			u32 j;
> +
> +			if (log_offset + num_logs > avail_logs) {
> +				xe_warn(xe, "[RAS]: CPER: header[%u] claims %u logs but only %u remain\n",
> +					i, num_logs, avail_logs - log_offset);
> +				break;
> +			}
> +
> +			if (!num_logs) {
> +				log_offset += num_logs;
> +				continue;
> +			}

[Severity: Medium]
If `num_logs` is 0, this branch continues without initializing the header data
for `einfo_arr[i]`. Does this leave an uninitialized gap in the `einfo_arr`
array that gets exposed to the caller, leading to the emission of malformed
empty CPER records?

> +
> +			einfo_size = sizeof(*einfo) + num_logs * entry_size;
> +			einfo = kzalloc(einfo_size, GFP_KERNEL);
[ ... ]
> +			einfo_arr[i].hdr        = hdrs[i];
> +			einfo_arr[i].einfo      = einfo;
> +			einfo_arr[i].einfo_size = einfo_size;
> +			log_offset += num_logs;
> +		}
> +
> +		*count_out = i;

[Severity: Medium]
If the loop encounters headers where `num_logs == 0`, `*count_out` is still
set to the total number of headers processed (including the skipped ones).
Could this cause the caller to consume the sparse, zero-filled array entries
that were skipped earlier?

> +	}
> +
> +	kfree(raw_buf);
> +	return einfo_arr;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825175916.1103841-13-badal.nilawar@intel.com?part=5