Re: [PATCH 3/6] gpu: host1x: Fix offset calculation in trace_write_gather

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Integer overflow in `copy_gathers` leads to a massive heap buffer overflow
- [High] Mismatched buffer object and offset in `trace_write_gather` causes out-of-bounds read and information leak
- [High] Trace event uses fixed-size scalar field for dynamic string, causing use-after-free
- [High] Missing error pointer check for `host1x_bo_mmap()` leads to kernel oops on failure
--

commit 32809268254f182a4349aa6566cab30f99b736b0
Author: Mikko Perttunen <mperttunen@nvidia.com>

gpu: host1x: Fix offset calculation in trace_write_gather

When a gather longer than 2*TRACE_MAX_LENGTH (256) words is traced
through host1x_cdma_push_gather, the reported BO offset drifts from
the third iteration onward.

[Severity: Critical]
This is a pre-existing issue, but does copy_gathers() in
drivers/gpu/host1x/job.c have an integer overflow when accumulating sizes?

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
    size += g->words * sizeof(u32);
    ...
}

If userspace submits a job with enough commands, could size wrap around on
32-bit systems? 

This wrapped size would then be used by dma_alloc_wc() to allocate a small
buffer, followed by a loop that copies a large amount of data:

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
    memcpy(job->gather_copy_mapped + offset, gather + g->offset,
           g->words * sizeof(u32));
    ...
}

Could this lead to a large heap buffer overflow?

[Severity: High]
This isn't a bug introduced by this patch, but looking at the trace event
definition for host1x_cdma_push_gather:

include/trace/events/host1x.h:
    __field(const char *, name)

According to trace subsystem rules, dynamic-length strings should be stored
using __string() and __assign_str() to capture the string contents. By using
a pointer field, only the address is captured.

If the trace buffer is read after the device has been unregistered and its
name freed, could this pointer dangle and cause a use-after-free when
formatting the trace?

[Severity: High]
This is a pre-existing issue, but I noticed host1x_bo_mmap() can return an
error pointer (e.g. ERR_PTR(-ENOMEM) from tegra_bo_mmap()).

In trace_write_gather() earlier in this file:

    if (host1x_debug_trace_cmdbuf)
        mem = host1x_bo_mmap(bo);
    
    if (mem) {

Is it possible for mem to be an error pointer here? If so, checking if (mem)
will treat the error pointer as a valid address.

There appear to be similar checks elsewhere that might not catch error
pointers:

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
    gather = host1x_bo_mmap(g->bo);
    memcpy(job->gather_copy_mapped + offset, gather + g->offset,
           g->words * sizeof(u32));
    ...
}

drivers/gpu/host1x/job.c:do_relocs() {
    ...
    cmdbuf_addr = host1x_bo_mmap(cmdbuf);

    if (unlikely(!cmdbuf_addr)) {
    ...
}

Could these lead to a kernel oops if host1x_bo_mmap() fails and returns an
error pointer instead of NULL?

> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
> index 2df6a16d484e0..9dda73199889c 100644
> --- a/drivers/gpu/host1x/hw/channel_hw.c
> +++ b/drivers/gpu/host1x/hw/channel_hw.c

[ ... ]

> @@ -36,10 +36,9 @@ static void trace_write_gather(struct host1x_cdma *cdma, struct host1x_bo *bo,

[Severity: High]
This is a pre-existing issue, but is there a mismatched buffer object and
offset passed to trace_write_gather()?

When the firewall is enabled, copy_gathers() combines all gathers into a
single DMA buffer and overwrites g->offset with the cumulative byte offset,
but leaves g->bo pointing to the original buffer:

drivers/gpu/host1x/job.c:copy_gathers() {
    ...
    g->base = job->gather_copy;
    g->offset = offset;
    ...
}

Then submit_gathers() passes these to trace_write_gather():

drivers/gpu/host1x/hw/channel_hw.c:submit_gathers() {
    ...
    trace_write_gather(cdma, g->bo, g->offset, g->words);
    ...
}

Since trace_write_gather() maps the original BO but uses the new combined
offset:

>  		for (i = 0; i < words; i += TRACE_MAX_LENGTH) {
>  			u32 num_words = min(words - i, TRACE_MAX_LENGTH);
>  
> -			offset += i * sizeof(u32);
> -
>  			trace_host1x_cdma_push_gather(dev_name(dev), bo,
> -						      num_words, offset,
> +						      num_words,
> +						      offset + i * sizeof(u32),
>  						      mem);
>  		}
>  

Will this cause trace_host1x_cdma_push_gather() to read out of bounds from
the mapped original BO using the large combined offset?

include/trace/events/host1x.h:host1x_cdma_push_gather() {
    ...
    memcpy(__get_dynamic_array(cmdbuf), cmdbuf+offset,
           words * sizeof(u32));
    ...
}

Could this leak kernel data to the trace buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-b4-host1x-small-fixes-a-v1-0-7c1131c0b3ad@nvidia.com?part=3