Re: [PATCH v2 06/11] drm/xe/cper: Log CPER records for aggregate counter retrival

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [Medium] Unconditional CPER record emission on counter retrieval causes log spam and zero-error records.
- [Medium] Spurious CPER records with empty metadata are emitted for info queue headers with zero logs.

Pre-existing issues:
- [Critical] An integer overflow in the bounds check `chunk_offset + chunk_size` allows firmware to perform an out-of-bounds write relative to the `raw_buf` heap allocation.
- [High] Missing bounds check on `chunk_size` against `XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE` allows a stack buffer over-read, leaking sensitive kernel stack data.
- [High] The do-while loop fetching info queue chunks can be forced into an infinite loop by malicious firmware, causing a kernel lockup.
--

commit 07283a153280cbc84316c197325268cbc06ecfe6
Author: Badal Nilawar <badal.nilawar@intel.com>

drm/xe/cper: Log CPER records for aggregate counter retrival
    
Log CPER records for aggregate counter retrieval from userspace.

> diff --git a/drivers/gpu/drm/xe/xe_ras.c b/drivers/gpu/drm/xe/xe_ras.c
> index 27c78800b5d27..ff9d917b8e29c 100644
> --- a/drivers/gpu/drm/xe/xe_ras.c
> +++ b/drivers/gpu/drm/xe/xe_ras.c
[ ... ]
> @@ -749,6 +777,66 @@ prepare_cper_error_info(struct xe_device *xe,
>  	return einfo_arr;
>  }
>  
> +static void emit_hw_error_cper(struct xe_device *xe,
> +			       struct xe_ras_error_class *error_class,
> +			       struct xe_ras_get_counter_response *resp,
> +			       u32 sig_id, u8 severity)
> +{
[ ... ]
> +	if (einfo_count > 0) {
> +		for (i = 0; i < einfo_count; i++) {
> +			struct xe_cper_sec_intel_err_hdr entry_ihdr = {};
> +
> +			xe_cper_init_intel_err_hdr(xe,
> +						   (const u8 *)&einfo_arr[i].hdr.error_class,

[Severity: Medium]
When prepare_cper_error_info() processes an info queue header with 0 logs, it
skips initializing einfo_arr[i] but still includes it in the *count_out total.
Does this loop unconditionally access and emit CPER records for potentially
uninitialized (zeroed) elements of the einfo_arr array?

[ ... ]
> +	} else {
> +		xe_cper_init_intel_err_hdr(xe,
> +					   (const u8 *)error_class,
> +					   counter_response->timestamp,
> +					   sig_id,
> +					   counter_response->value,
> +					   &ihdr);
> +		xe_cper_record_emit(xe, severity, &INTEL_CPER_NOTIFY_GPU_ERROR,
> +				    &ihdr, NULL, 0);
> +	}

[Severity: Medium]
If counter_response->value is 0 and there is no info queue, will this fallback
path explicitly emit a CPER record reporting 0 errors?

[ ... ]
> @@ -886,6 +974,10 @@ int xe_ras_get_counter(struct xe_device *xe, u8 severity, u8 component, u32 *val
>  		return ret;
>  
>  	*value = response.value;
> +
> +	emit_hw_error_cper(xe, &counter, &response,
> +			   ras_comp_to_hw_sigid(counter.common.component),
> +			   ras_sev_to_cper_sev(counter.common.severity));
>  	return 0;
>  }

[Severity: Medium]
Does this unconditional call trigger a CPER emission on every userspace read,
regardless of the counter's value or whether it has changed? This might lead
to log spam and incorrect zero-error telemetry when the counter is 0.

While reviewing this patch, I also noticed some pre-existing issues in the
nearby prepare_cper_error_info() function:

> 	if (first_qhdr->chunk_size &&
> 	    first_qhdr->chunk_offset + first_qhdr->chunk_size <=
> 	    XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE) {

[Severity: Critical]
This is a pre-existing issue, but is this boundary validation vulnerable to an
integer overflow? If the firmware provides a large chunk_offset (e.g.
0xFFFFFFF0) and a small chunk_size (e.g. 0x20), the sum could wrap around and
bypass the check, potentially allowing an out-of-bounds write relative to the
raw_buf heap allocation.

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

[Severity: High]
This is a pre-existing issue, but does this memcpy lack a bounds check on
chunk_size against the stack-allocated queue_data array bounds
(XE_RAS_INFO_QUEUE_MAX_CHUNK_SIZE)? If firmware provides a large chunk_size
that passes the total boundary check, could this read past the end of the array
and leak sensitive kernel stack memory into the heap buffer?

[ ... ]
> 		do {
> 			struct xe_ras_info_queue_header *qhdr;
> 			u32 end;
[ ... ]
> 			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]
This is a pre-existing issue, but similar to the first chunk, is this
subsequent boundary validation also vulnerable to an integer overflow if
chunk_offset and chunk_size wrap around?

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

[Severity: High]
This is a pre-existing issue, but does this location also lack a bounds check
on chunk_size against the size of the iq_response stack variable, potentially
allowing a stack buffer over-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]
This is a pre-existing issue, but can this do-while loop be forced into an
infinite loop by malicious or unresponsive firmware? If the firmware repeatedly
returns a valid chunk_offset and chunk_size with the MORE_DATA flag set such
that the calculated end boundary never exceeds the limit, could this cause a
kernel soft lockup?

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