Re: [PATCH v3 05/12] drm/xe/xe_ras: Add support to retrieve info queue data for CRI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Inconsistent handling of queue offsets and blindly trusting the hardware's total_size leads to sparse buffer writes and returning an inflated valid byte count.
- [High] Missing contiguous progress validation allows a misbehaving or compromised firmware to cause an infinite loop in the kernel.
- [Medium] get_info_queue_data ignores the hardware operation_status, returning success on logical command failures.
--

commit beae2ef0daf0b18e6a50c5076d39abd256ed8593
Author: Badal Nilawar <badal.nilawar@intel.com>
Subject: drm/xe/xe_ras: Add support to retrieve info queue data for CRI

Retrieve the RAS info queue data, in multiple chunks, and assemble
it into flat raw buffer. Follow up patch will use this data to
prepare cper error info.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 0fb9065cdd76f..7e3e627504485 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> +static int get_info_queue_data(struct xe_device *xe,
> +			       const struct xe_ras_get_info_queue_data_request *req,
> +			       struct xe_ras_get_info_queue_data_response *out)
> +{
[ ... ]
> +	if (rlen != sizeof(response)) {
> +		xe_err(xe, "sysctrl: unexpected get info queue data response length %zu (expected %zu)\n",
> +		       rlen, sizeof(response));
> +		return -EIO;
> +	}
> +
> +	xe_dbg(xe, "[RAS]: info queue data: status=%u chunk_size=%u flags=0x%x\n",
> +	       response.operation_status,
> +	       response.queue_response.queue_header.chunk_size,
> +	       response.queue_response.queue_header.flags);
> +
> +	*out = response;
> +	return 0;
> +}

[Severity: Medium]
Does get_info_queue_data() ignore the hardware operation_status? It looks like
it returns success on logical command failures. If the GET_INFO_QUEUE_DATA
command fails at the firmware level, returning an error code in
response.operation_status, the function only checks the transport status and
returns 0. Can this cause the driver to process an empty or garbage
queue_response payload as valid data?

[ ... ]
> +u32 xe_ras_drain_info_queue_raw(struct xe_device *xe,
> +				const struct xe_ras_get_counter_response *counter_resp,
> +				u8 *raw_buf, u32 raw_buf_size)
> +{
[ ... ]
> +	if (!raw_buf || !raw_buf_size)
> +		return 0;
> +
> +	/* Copy first chunk already embedded in the counter response */
> +	if (first_qhdr->chunk_size &&
> +	    first_qhdr->chunk_size <= XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE &&
> +	    !check_add_overflow(first_qhdr->chunk_offset, first_qhdr->chunk_size, &end) &&
> +	    end <= XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE && end <= raw_buf_size) {
> +		memcpy(raw_buf + first_qhdr->chunk_offset,
> +		       counter_resp->info_queue.queue_data, first_qhdr->chunk_size);
> +		iq_offset = first_qhdr->chunk_size;
> +	}

[Severity: High]
Does the initialization of iq_offset incorrectly assume that the first chunk's
offset is exactly 0? Also, if the bounds check fails, the copy is skipped but
the complete variable is left as true. Could this result in miscalculating
iq_offset (e.g., iq_offset = chunk_size instead of chunk_offset + chunk_size)
and returning an inflated valid byte count?

> +
> +	/* Fetch any remaining chunks */
> +	if (first_qhdr->flags & XE_RAS_INFO_QUEUE_FLAG_MORE_DATA) {
[ ... ]
> +			if (check_add_overflow(qhdr->chunk_offset, qhdr->chunk_size, &end) ||
> +			    end > XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE || end > raw_buf_size) {
> +				complete = false;
> +				xe_warn(xe,
> +					"[RAS]: info queue chunk out of bounds (offset=%u size=%u)\n",
> +					qhdr->chunk_offset, qhdr->chunk_size);
> +				break;
> +			}

[Severity: High]
The bounds check uses qhdr->chunk_offset but lacks a check like
qhdr->chunk_offset == iq_offset to guarantee monotonic forward progress.
Could this missing contiguous progress validation allow a misbehaving or
compromised firmware to cause an infinite loop in the kernel? Firmware could
repeatedly return a valid small chunk (e.g., chunk_offset = 0, chunk_size = 1)
with the MORE_DATA flag set. The bounds check would pass every time, and
iq_offset would grow indefinitely without terminating the loop.

> +
> +			memcpy(raw_buf + qhdr->chunk_offset,
> +			       iq_response.queue_response.queue_data,
> +			       qhdr->chunk_size);
> +
> +			iq_offset += qhdr->chunk_size;

[Severity: High]
Does this missing contiguous offset validation (e.g.
qhdr->chunk_offset == iq_offset) allow uninitialized gaps in the destination
buffer, and blindly advance the requested offset? This could lead to the caller
parsing zeroed or uninitialized memory as valid CPER error logs, resulting
in corrupted error reports if firmware returns chunks with gaps.

> +		} while (iq_response.queue_response.queue_header.flags &
> +			 XE_RAS_INFO_QUEUE_FLAG_MORE_DATA);

[Severity: High]
Does the loop rely entirely on the untrusted hardware clearing the MORE_DATA
flag to terminate, without any software fallback limiting total iterations or
checking accumulated size?

> +	}
> +
> +	if (!complete)
> +		return iq_offset;
> +
> +	return first_qhdr->total_size
> +	       ? min3(first_qhdr->total_size, XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE, raw_buf_size)
> +	       : iq_offset;
> +}

[Severity: High]
Is it safe to return an unverified hardware-provided total_size instead of the
dynamically accumulated written byte count? If firmware returns
MORE_DATA == 0 early or returns chunks with gaps, this exposes uninitialized
memory at the tail of the buffer.

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