Re: [PATCH v8 2/2] drm/xe/guc: Compress GuC log and CTB dump with zstd

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check in `xe_guc_log_load_lic` when parsing untrusted GuC log.
--

commit 91217fd983f065c8684178f4aa349f4bc46ae614
Author: Zhanjun Dong <zhanjun.dong@intel.com>
Subject: drm/xe/guc: Compress GuC log and CTB dump with zstd

With default settings, GuC log is 8MB in size and dumping the log is
time-consuming. In debugging scenarios, dump-to-dmesg suffers truncation loss
because CI's size limit is 2MB. Thus, compress the GuC log data dump to
address these issues and multiply any benefits from a future upgrade in CI's
size limits.

> diff --git a/drivers/gpu/drm/xe/xe_guc_log.c b/drivers/gpu/drm/xe/xe_guc_log.c
> index 538d4df0f7aa7..aaeb0f76af305 100644
> --- a/drivers/gpu/drm/xe/xe_guc_log.c
> +++ b/drivers/gpu/drm/xe/xe_guc_log.c

[ ... ]

> @@ -248,16 +383,9 @@ void xe_guc_log_snapshot_print(struct xe_guc_log_snapshot *snapshot, struct drm_
>  	drm_printf(p, "GuC timestamp: 0x%08llX [%llu]\n", snapshot->stamp, snapshot->stamp);
>  	drm_printf(p, "Log level: %u\n", snapshot->level);
>  
> -	drm_printf(p, "[LOG].length: 0x%zx\n", snapshot->size);
> -	remain = snapshot->size;
> -	for (i = 0; i < snapshot->num_chunks; i++) {
> -		size_t size = min(GUC_LOG_CHUNK_SIZE, remain);
> -		const char *prefix = i ? NULL : "[LOG].data";
> -		char suffix = i == snapshot->num_chunks - 1 ? '\n' : 0;
> -
> -		xe_print_blob_ascii85(p, prefix, suffix, snapshot->copy[i], 0, size);
> -		remain -= size;
> -	}
> +	xe_guc_print_blob(gt, p, "[LOG]", "[LOG].data",
> +			  snapshot->copy, snapshot->num_chunks,
> +			  GUC_LOG_CHUNK_SIZE, snapshot->size);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does xe_guc_log_load_lic()
have a missing bounds check when parsing an untrusted GuC log?

The function xe_guc_log_load_lic() parses an untrusted log buffer mapped
from the GuC firmware:

drivers/gpu/drm/xe/xe_guc_log.c:xe_guc_log_load_lic() {
	while (p->marker[0]) {
		int index;
		...
		offset += p->size;
		p++;
	}
}

The loop unconditionally increments p without verifying that it remains
within the bounds of the snapshot->copy[0] buffer. If the firmware buffer
is corrupted, malicious, or lacks the zero marker, the loop will increment
p past the end of the buffer (which is limited to GUC_LOG_CHUNK_SIZE).

Can this cause an out-of-bounds dereference on p->marker[0] and subsequent
fields? Since this is reachable via user-space by reading the GuC log debugfs
file (which triggers xe_guc_log_print_lfd() -> xe_guc_log_load_lic()), a
compromised or buggy firmware could potentially cause a local DoS or system
crash.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902224417.879960-4-alan.previn.teres.alexis@intel.com?part=2