Re: [PATCH v3 06/12] drm/xe/cper: Prepare Intel CPER error info records

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incorrect parsing of aggregate info queue logs causes offset corruption and truncated error counts.

Pre-existing issues:
- [High] Inverted hardware SIGID check causes the hardware error handler to unconditionally abort.
- [High] Unbounded loop in `xe_ras_drain_info_queue_raw` leading to Denial of Service.
--

commit 08c625ea20b49a15a86e85dcc258d1ff791d3145
Author: Badal Nilawar <badal.nilawar@intel.com>
drm/xe/cper: Prepare Intel CPER error info records

This patch prepares xe_cper_sec_intel_error_info records from RAS info
queue data. Depending on the error class type, one or more error info
records are generated and embedded in the CPER record.

> diff --git a/drivers/gpu/drm/xe/xe_cper.c b/drivers/gpu/drm/xe/xe_cper.c
> --- a/drivers/gpu/drm/xe/xe_cper.c
> +++ b/drivers/gpu/drm/xe/xe_cper.c
[ ... ]
> +static struct xe_cper_einfo_entry *
> +xe_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;
> +
> +	raw_total = xe_ras_drain_info_queue_raw(xe, counter_resp, raw_buf,
> +						XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE);

[Severity: High]
This is a pre-existing issue, but since this patch adds a call to it here,
can xe_ras_drain_info_queue_raw() cause a denial of service if the
hardware misbehaves?

Looking at xe_ras_drain_info_queue_raw() in drivers/gpu/drm/xe/xe_ras.c,
the loop bounds check evaluates the received chunk offset:

    if (check_add_overflow(qhdr->chunk_offset, qhdr->chunk_size, &end) ||
        end > XE_RAS_INFO_QUEUE_MAX_TOTAL_SIZE || end > raw_buf_size) {

However, the loop termination relies entirely on the MORE_DATA flag:

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

It never checks if the accumulated iq_offset exceeds the total maximum
limit across iterations. Could a malfunctioning or malicious firmware
hang the kernel by continuously returning valid small chunks with the
MORE_DATA flag set?

[ ... ]
> +		for (i = 0; i < num_headers; i++) {
> +			u32 num_logs = min_t(u32, hdrs[i].counter, XE_RAS_NUM_COUNTERS);
> +			struct xe_cper_sec_intel_error_info *einfo;
> +
> +			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)
> +				continue;
> +
> +			einfo = build_einfo(&all_logs[log_offset], num_logs,
> +					    &hdrs[i].error_class, num_logs,
> +					    &einfo_arr[einfo_count].einfo_size,
> +					    &einfo_arr[einfo_count].timestamp);

[Severity: High]
Does clamping num_logs to XE_RAS_NUM_COUNTERS here cause parsing
desynchronization?

If the hardware returns an aggregate info queue where hdrs[i].counter is
greater than XE_RAS_NUM_COUNTERS, num_logs is clamped. But this clamped
value is also passed to build_einfo() as the error_count, leading to
truncated error counts.

> +			if (!einfo) {
> +				free_einfo_arr(einfo_arr, einfo_count);
> +				kfree(raw_buf);
> +				return NULL;
> +			}
> +
> +			einfo_arr[einfo_count].hdr   = hdrs[i];
> +			einfo_arr[einfo_count].einfo = einfo;
> +			log_offset += num_logs;

[Severity: High]
Additionally, does incrementing log_offset by the clamped amount rather
than the claimed amount cause subsequent headers to read the wrong log
data? It seems like it would leave unconsumed logs in the stream,
corrupting the offset for the next iteration.

[ ... ]
>  void xe_emit_hardware_error_cper(struct pci_dev *pdev, int cper_sev, enum xe_sigid sigid,
>  				 struct xe_ras_error_class *counter,
>  				 struct xe_ras_get_counter_response *response)
>  {
>  	struct xe_device *xe = pdev_to_xe_device(pdev);
>  	struct xe_ras_get_counter_response local_resp = {};
>  	struct xe_ras_get_counter_response *counter_response = response;
>  	struct xe_cper_sec_intel_err_hdr ihdr = {};
> +	struct xe_cper_einfo_entry *einfo_arr = NULL;
> +	u32 einfo_count = 0;
> +	u32 i;
>  
>  	if (!xe)
>  		return;
>  
>  	if ((int)sigid >= INTEL_SIGID_GPU_XE_HARDWARE_START)
>  		return;

[Severity: High]
This is a pre-existing issue, but is this check inverted?

It aborts if the error is >= INTEL_SIGID_GPU_XE_HARDWARE_START, which
seems to dead-code the entire hardware error logging feature. Since this
occurs on every hardware error, does this check completely neutralize
xe_emit_hardware_error_cper() for hardware errors?

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