[PATCH] drm/imagination: clamp freelist reconstruction requests

[PATCH] drm/imagination: clamp freelist reconstruction requests

[Pengpeng Hou]

The firmware reconstruction request contains a count followed by a
fixed array of ROGUE_FWIF_MAX_FREELISTS_TO_RECONSTRUCT IDs. The host
uses the count to walk the request array and to copy IDs into an equally
sized response array without checking the protocol maximum.

A firmware count above that maximum therefore makes both operations
access beyond their protocol objects. Clamp the count to the array
capacity, reconstruct only that bounded subset, and report the same
bounded count to firmware. This follows the recovery contract confirmed
by the firmware maintainers: an incomplete bounded update is preferable
to an unrepresentable response and gives firmware a chance to recover.

Warn once because an oversized count is a firmware bug, while avoiding
repeated log flooding if firmware retries the malformed request.

Fixes: 6eedddab733b ("drm/imagination: Implement free list and HWRT create and destroy ioctls")
Assisted-by: Codex:gpt-5
Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since the RFC: https://lore.kernel.org/all/20260722040942.85459-1-pengpeng@iscas.ac.cn/
- implement the maintainer-confirmed bounded recovery policy
- warn once when firmware exceeds the protocol array capacity
- disclose the use of Codex

The protocol extents and recovery path were reviewed statically. The
change was not tested on PowerVR hardware.

 drivers/gpu/drm/imagination/pvr_free_list.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_free_list.c b/drivers/gpu/drm/imagination/pvr_free_list.c
index e85cac83834c..23dbc227b081 100644
--- a/drivers/gpu/drm/imagination/pvr_free_list.c
+++ b/drivers/gpu/drm/imagination/pvr_free_list.c
@@ -612,13 +612,20 @@ pvr_free_list_process_reconstruct_req(struct pvr_device *pvr_dev,
 	};
 	struct rogue_fwif_freelists_reconstruction_data *resp =
 		&resp_cmd.cmd_data.free_lists_reconstruction_data;
+	u32 count = min_t(u32, req->freelist_count,
+			  ARRAY_SIZE(req->freelist_ids));
 
-	for (u32 i = 0; i < req->freelist_count; i++)
+	if (count != req->freelist_count)
+		drm_warn_once(from_pvr_device(pvr_dev),
+			      "Firmware requested reconstruction of %u freelists, limiting to %u\n",
+			      req->freelist_count, count);
+
+	for (u32 i = 0; i < count; i++)
 		pvr_free_list_reconstruct(pvr_dev, req->freelist_ids[i]);
 
-	resp->freelist_count = req->freelist_count;
+	resp->freelist_count = count;
 	memcpy(resp->freelist_ids, req->freelist_ids,
-	       req->freelist_count * sizeof(resp->freelist_ids[0]));
+	       count * sizeof(resp->freelist_ids[0]));
 
 	WARN_ON(pvr_kccb_send_cmd(pvr_dev, &resp_cmd, NULL));
 }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] drm/imagination: clamp freelist reconstruction requests

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized kernel stack memory (union padding and trailing array elements) sent to firmware CCB (Command Control Buffer).
--

--- Patch [1]: [PATCH] drm/imagination: clamp freelist reconstruction requests ---
commit ba7e433ffb1bb57e684515b2def3023d1d0aff07
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

drm/imagination: clamp freelist reconstruction requests

This commit bounds the freelist reconstruction count from firmware to the
maximum array capacity to prevent out-of-bounds accesses. It processes
only the bounded subset and reports this bounded count back to firmware,
emitting a one-time warning if the limit is exceeded.

> diff --git a/drivers/gpu/drm/imagination/pvr_free_list.c b/drivers/gpu/drm/imagination/pvr_free_list.c
> index e85cac83834c6..23dbc227b081a 100644
> --- a/drivers/gpu/drm/imagination/pvr_free_list.c
> +++ b/drivers/gpu/drm/imagination/pvr_free_list.c
> @@ -612,13 +612,20 @@ pvr_free_list_process_reconstruct_req(struct pvr_device *pvr_dev,
>  	};
>  	struct rogue_fwif_freelists_reconstruction_data *resp =
>  		&resp_cmd.cmd_data.free_lists_reconstruction_data;

[Severity: High]
This isn't a bug introduced by this patch, but does this initialization leave
union padding uninitialized?

The resp_cmd struct uses a designated initializer for cmd_type. This
zero-initializes only the first member of its cmd_data union (cmd_kick_data),
leaving the remainder of the union uninitialized.

> +	u32 count = min_t(u32, req->freelist_count,
> +			  ARRAY_SIZE(req->freelist_ids));
>  
> -	for (u32 i = 0; i < req->freelist_count; i++)
> +	if (count != req->freelist_count)
> +		drm_warn_once(from_pvr_device(pvr_dev),
> +			      "Firmware requested reconstruction of %u freelists, limiting to %u\n",
> +			      req->freelist_count, count);
> +
> +	for (u32 i = 0; i < count; i++)
>  		pvr_free_list_reconstruct(pvr_dev, req->freelist_ids[i]);
>  
> -	resp->freelist_count = req->freelist_count;
> +	resp->freelist_count = count;
>  	memcpy(resp->freelist_ids, req->freelist_ids,
> -	       req->freelist_count * sizeof(resp->freelist_ids[0]));
> +	       count * sizeof(resp->freelist_ids[0]));

If count is small (e.g., 1), the memcpy only fills count elements of the array.
The trailing elements of resp->freelist_ids will fall into the uninitialized
union padding.

>  
>  	WARN_ON(pvr_kccb_send_cmd(pvr_dev, &resp_cmd, NULL));
>  }

When pvr_kccb_send_cmd() copies the full sizeof(struct rogue_fwif_kccb_cmd) to
the KCCB ring buffer, could it leak this uninitialized kernel stack memory to
the GPU firmware?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813152759.35856-1-pengpeng@iscas.ac.cn?part=1