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 <[email protected]> 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/[email protected]?part=1
