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

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 <[email protected]>

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