Re: [PATCH] gpu: host1x: skip trace payloads after mapping failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] In `trace_write_gather()`, the `offset` variable incorrectly accumulates quadratically on each loop iteration, leading to an out-of-bounds read when tracing gathers larger than `TRACE_MAX_LENGTH`.
- [High] The patch fixes an ERR_PTR dereference for `host1x_bo_mmap()` in `trace_write_gather()`, but fails to fix identical vulnerabilities in three other call sites within the host1x subsystem.
--

commit 670e4c9b37822e7d842f4b22310017b4bf9a72f2
Author: Slavin Liu <bolin.liu@seu.edu.cn>

gpu: host1x: skip trace payloads after mapping failure

This patch updates the return check of host1x_bo_mmap() in trace_write_gather()
to use IS_ERR_OR_NULL instead of a simple NULL check. It avoids dereferencing
an ERR_PTR mapping when the original buffer mapping fails. While reviewing this
fix, I noticed some pre-existing issues in the same function and in other
callers of host1x_bo_mmap().

> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
> index d44b8de890be0..e00f257710bdd 100644
> --- a/drivers/gpu/host1x/hw/channel_hw.c
> +++ b/drivers/gpu/host1x/hw/channel_hw.c
> @@ -27,7 +27,7 @@ static void trace_write_gather(struct host1x_cdma *cdma, struct host1x_bo *bo,
>  	if (host1x_debug_trace_cmdbuf)
>  		mem = host1x_bo_mmap(bo);
>  
> -	if (mem) {
> +	if (!IS_ERR_OR_NULL(mem)) {
>  		u32 i;
>  		/*
>  		 * Write in batches of 128 as there seems to be a limit

[Severity: High]
This is a pre-existing issue, but does the loop inside this block incorrectly
accumulate the running loop index into the base offset?

In trace_write_gather():

		for (i = 0; i < words; i += TRACE_MAX_LENGTH) {
			u32 num_words = min(words - i, TRACE_MAX_LENGTH);

			offset += i * sizeof(u32);

Because i increases linearly on each iteration (0, 128, 256...), adding it
directly to offset in-place causes the offset to grow quadratically. If
words exceeds TRACE_MAX_LENGTH (128 words), could this overshoot the bounds
of the mapped buffer mem and lead to an out-of-bounds read in the ftrace
callback?

While this patch fixes the ERR_PTR dereference for host1x_bo_mmap() in
trace_write_gather(), it looks like there are similar pre-existing
vulnerabilities in three other call sites within the host1x subsystem.

In drivers/gpu/host1x/job.c:do_relocs():

		if (!cmdbuf_addr) {
			cmdbuf_addr = host1x_bo_mmap(cmdbuf);

			if (unlikely(!cmdbuf_addr)) {
				pr_err("Could not map cmdbuf for relocation\n");
				return -ENOMEM;
			}
		}

		target = cmdbuf_addr + reloc->cmdbuf.offset;

[Severity: High]
This is a pre-existing issue, but since host1x_bo_mmap() can return an ERR_PTR
on failure, will this NULL check allow an ERR_PTR to bypass the check and be
used as the base pointer for a relocation write?

In drivers/gpu/host1x/job.c:copy_gathers():

		/* Copy the gather */
		gather = host1x_bo_mmap(g->bo);
		memcpy(job->gather_copy_mapped + offset, gather + g->offset,
		       g->words * sizeof(u32));

[Severity: High]
This is a pre-existing issue, but could the lack of error checking here cause
memcpy() to directly dereference an ERR_PTR if the mapping fails?

In drivers/gpu/host1x/hw/debug_hw.c:show_channel_gathers():

			if (job->gather_copy_mapped)
				mapped = (u32 *)job->gather_copy_mapped;
			else
				mapped = host1x_bo_mmap(g->bo);

			if (!mapped) {
				host1x_debug_output(o, "[could not mmap]\n");
				continue;
			}

[Severity: High]
This is a pre-existing issue, but might this NULL check fail to catch an
ERR_PTR, resulting in an out-of-bounds read when show_gather() dereferences
the mapped pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260913125128.109827-1-bolin.liu@seu.edu.cn?part=1