[PATCH 0/2] drm/imagination: Firmware interface cleanup and improvements

[PATCH 0/2] drm/imagination: Firmware interface cleanup and improvements

[Alexandru Dadu]

Series contains cleanup commits targeted to FW interface.
    - Avoid initialization of never used data.
    - KCCB member initialized in the correct place.

Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
Alessio Belle (1):
      drm/imagination: Avoid initialisation of unused FW trace buffer pointer

Matt Coster (1):
      drm/imagination: Collect KCCB initialisation

 drivers/gpu/drm/imagination/pvr_ccb.c      | 38 ++++++++++++++++++++++++++----
 drivers/gpu/drm/imagination/pvr_fw.c       | 17 +------------
 drivers/gpu/drm/imagination/pvr_fw_trace.c |  1 -
 3 files changed, 35 insertions(+), 21 deletions(-)
---
base-commit: bd4f284df04d76fd65e57141cb1e6e7a49e4c3cb
change-id: 20260909-b4-avoid-init-of-unused-fw-trace-buffer-pointer-ad76e5b57945

Best regards,
--  
Alexandru Dadu <alexandru.dadu@imgtec.com>

[PATCH 1/2] drm/imagination: Collect KCCB initialisation

[Alexandru Dadu]

From: Matt Coster <matt.coster@imgtec.com>

There's one FW object attached to KCCB usage (the return buffer) that is
separately initialised in pvr_fw_init(). Move this initialisation to
pvr_kccb_init() alongside the initialisation of rest of the members of
struct pvr_device->kccb.

Signed-off-by: Matt Coster <matt.coster@imgtec.com>
Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_ccb.c | 38 +++++++++++++++++++++++++++++++----
 drivers/gpu/drm/imagination/pvr_fw.c  | 17 +---------------
 2 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_ccb.c b/drivers/gpu/drm/imagination/pvr_ccb.c
index b702d122d791..3b5942b674be 100644
--- a/drivers/gpu/drm/imagination/pvr_ccb.c
+++ b/drivers/gpu/drm/imagination/pvr_ccb.c
@@ -17,6 +17,7 @@
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
 #include <linux/mutex.h>
+#include <linux/overflow.h>
 #include <linux/types.h>
 #include <linux/workqueue.h>
 
@@ -527,6 +528,7 @@ void pvr_kccb_wake_up_waiters(struct pvr_device *pvr_dev)
  */
 void pvr_kccb_fini(struct pvr_device *pvr_dev)
 {
+	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
 	pvr_ccb_fini(&pvr_dev->kccb.ccb);
 	WARN_ON(!list_empty(&pvr_dev->kccb.waiters));
 	WARN_ON(pvr_dev->kccb.reserved_count);
@@ -543,14 +545,42 @@ void pvr_kccb_fini(struct pvr_device *pvr_dev)
 int
 pvr_kccb_init(struct pvr_device *pvr_dev)
 {
-	pvr_dev->kccb.slot_count = 1 << ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
+	const u32 num_slots_log2 = ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
+	const u32 num_slots = 1 << num_slots_log2;
+	u32 rtn_size;
+	int err;
+
+	/*
+	 * The inputs here are compile-time constants; there's no reason to try
+	 * to gracefully handle overflow at runtime.
+	 */
+	BUILD_BUG_ON(check_mul_overflow(num_slots, sizeof(*pvr_dev->kccb.rtn), &rtn_size));
+
+	pvr_dev->kccb.slot_count = num_slots;
 	INIT_LIST_HEAD(&pvr_dev->kccb.waiters);
 	pvr_dev->kccb.fence_ctx.id = dma_fence_context_alloc(1);
 	spin_lock_init(&pvr_dev->kccb.fence_ctx.lock);
 
-	return pvr_ccb_init(pvr_dev, &pvr_dev->kccb.ccb,
-			    ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT,
-			    sizeof(struct rogue_fwif_kccb_cmd));
+	err = pvr_ccb_init(pvr_dev, &pvr_dev->kccb.ccb, num_slots_log2,
+			   sizeof(struct rogue_fwif_kccb_cmd));
+	if (err)
+		return err;
+
+	/* Allocate memory for KCCB return slots. */
+	pvr_dev->kccb.rtn = pvr_fw_object_create_and_map(pvr_dev, rtn_size,
+							 PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
+							 NULL, NULL, &pvr_dev->kccb.rtn_obj);
+	if (IS_ERR(pvr_dev->kccb.rtn)) {
+		err = PTR_ERR(pvr_dev->kccb.rtn);
+		goto err_ccb_fini;
+	}
+
+	return 0;
+
+err_ccb_fini:
+	pvr_ccb_fini(&pvr_dev->kccb.ccb);
+
+	return err;
 }
 
 /**
diff --git a/drivers/gpu/drm/imagination/pvr_fw.c b/drivers/gpu/drm/imagination/pvr_fw.c
index 850a3ec8e775..cec17352cf90 100644
--- a/drivers/gpu/drm/imagination/pvr_fw.c
+++ b/drivers/gpu/drm/imagination/pvr_fw.c
@@ -945,8 +945,6 @@ pvr_fw_init(struct pvr_device *pvr_dev)
 		[PVR_FW_PROCESSOR_TYPE_RISCV] = &pvr_fw_defs_riscv,
 	};
 
-	u32 kccb_size_log2 = ROGUE_FWIF_KCCB_NUMCMDS_LOG2_DEFAULT;
-	u32 kccb_rtn_size = (1 << kccb_size_log2) * sizeof(*pvr_dev->kccb.rtn);
 	struct pvr_fw_device *fw_dev = &pvr_dev->fw_dev;
 	int err;
 
@@ -981,18 +979,9 @@ pvr_fw_init(struct pvr_device *pvr_dev)
 	if (err)
 		goto err_kccb_fini;
 
-	/* Allocate memory for KCCB return slots. */
-	pvr_dev->kccb.rtn = pvr_fw_object_create_and_map(pvr_dev, kccb_rtn_size,
-							 PVR_BO_FW_FLAGS_DEVICE_UNCACHED,
-							 NULL, NULL, &pvr_dev->kccb.rtn_obj);
-	if (IS_ERR(pvr_dev->kccb.rtn)) {
-		err = PTR_ERR(pvr_dev->kccb.rtn);
-		goto err_fwccb_fini;
-	}
-
 	err = pvr_fw_create_structures(pvr_dev);
 	if (err)
-		goto err_kccb_rtn_release;
+		goto err_fwccb_fini;
 
 	err = pvr_fw_start(pvr_dev);
 	if (err)
@@ -1014,9 +1003,6 @@ pvr_fw_init(struct pvr_device *pvr_dev)
 err_destroy_structures:
 	pvr_fw_destroy_structures(pvr_dev);
 
-err_kccb_rtn_release:
-	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
-
 err_fwccb_fini:
 	pvr_ccb_fini(&pvr_dev->fwccb);
 
@@ -1047,7 +1033,6 @@ pvr_fw_fini(struct pvr_device *pvr_dev)
 	WRITE_ONCE(fw_dev->initialised, false);
 
 	pvr_fw_destroy_structures(pvr_dev);
-	pvr_fw_object_unmap_and_destroy(pvr_dev->kccb.rtn_obj);
 
 	/*
 	 * Ensure FWCCB worker has finished executing before destroying FWCCB. The IRQ handler has

-- 
2.43.0

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

[Alexandru Dadu]

From: Alessio Belle <alessio.belle@imgtec.com>

This pointer, which is part of struct rogue_fwif_tracebuf_space (a FW
interface structure), was initialised but never used to access the FW
trace buffer - only the source pointer in the initialisation being
removed is used.

Signed-off-by: Alessio Belle <alessio.belle@imgtec.com>
Signed-off-by: Alexandru Dadu <alexandru.dadu@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_fw_trace.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/imagination/pvr_fw_trace.c b/drivers/gpu/drm/imagination/pvr_fw_trace.c
index 6bb5baa6c41b..813b7b0c8d4e 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;
 	}
 }

-- 
2.43.0

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

[sashiko-bot]

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