Re: [PATCH 2/2] drm/imagination: Avoid initialisation of unused FW trace buffer pointer

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the debugfs trace output formatter uses unbounded `%s` to print firmware-provided assertion strings, which can read past the end of the allocation if the strings lack a NUL terminator.
--

commit 97d5534fd4f9139da050fa97ad4093d575e6bdd3
Author: Alessio Belle <alessio.belle@imgtec.com>

drm/imagination: Avoid initialisation of unused FW trace buffer pointer

This commit removes the initialization of tracebuf_space->trace_buffer,
an unused pointer in the firmware interface structure. The trace buffer
source pointer continues to be used as before.

> diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> index 6bb5baa6c41b7..813b7b0c8d4e9 100644
> --- a/drivers/gpu/drm/imagination/pvr_fw_trace.c
> +++ b/drivers/gpu/drm/imagination/pvr_fw_trace.c
> @@ -101,7 +101,6 @@ tracebuf_ctrl_init(void *cpu_ptr, void *priv)
>  		pvr_fw_object_get_fw_addr(trace_buffer->buf_obj,
>  					  &tracebuf_space->trace_buffer_fw_addr);
>  
> -		tracebuf_space->trace_buffer = trace_buffer->buf;
>  		tracebuf_space->trace_pointer = 0;
>  	}
>  }

[Severity: High]
This isn't a bug introduced by this patch, but looking at another part of
pvr_fw_trace.c in fw_trace_seq_show(), the debugfs trace output formatter uses
unbounded %s format specifiers to print firmware-provided assertion strings:

    seq_printf(s, "ASSERTION %s failed at %s:%u",
               trace_seq_data->assert_buf.info,
               trace_seq_data->assert_buf.path,
               trace_seq_data->assert_buf.line_num);

Can this read past the end of the allocation if the firmware provides strings
without a NUL terminator?

Since assert_buf is the last member of trace_seq_data, reading past the end of
info or path could read out of the structure's bounds into adjacent heap memory.
If the unbounded string read crosses a page boundary into an unmapped page, it
could cause a kernel panic. Additionally, it might leak adjacent kernel heap
memory into the debugfs output.

Would it be safer to use %.*s with the maximum buffer size, or forcefully
NUL-terminate the buffers after copying them from the firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910-b4-avoid-init-of-unused-fw-trace-buffer-pointer-v1-0-7e10274384c2@imgtec.com?part=2