Re: [RFC] drm/pvr: define oversized freelist reconstruction handling

[Alessio Belle <Alessio.Belle@imgtec.com>]

Hi Pengpeng,

Sorry for the delay and thanks for raising this. A few comments below.

On Wed, 2026-07-22 at 12:09 +0800, Pengpeng Hou wrote:
> pvr_free_list_process_reconstruct_req() receives a reconstruction count
> from firmware and uses it to iterate the request ID array and populate the
> response ID array. Both arrays have
> ROGUE_FWIF_MAX_FREELISTS_TO_RECONSTRUCT entries, but the count is not
> checked against that limit.

ROGUE_FWIF_MAX_FREELISTS_TO_RECONSTRUCT is meant to be the maximum number of
freelists in use at any time plus the ones being setup. If the firmware did send
a count above the maximum, it should be considered a bug.

> 
> An oversized count would therefore make the loop and memcpy() access past
> the fixed protocol objects. The bounds issue is clear, but I could not find
> an ABI-defined completion for a malformed whole request.
> 
> In particular, clamping does not appear justified. It changes the response
> count and silently omits IDs that firmware claims to have supplied. Dropping
> the request may leave firmware waiting for the reconstruction update, while
> echoing the original count cannot be represented safely in the fixed
> response.

You are right that sending a response that is too long is not safe, in fact
reading a request that is too long or malformed is also not safe in general.

Sending an incomplete reconstruction update (i.e. clamping to the expected
maximum), where each freelist has been marked as reconstructed, will still allow
the firmware to at least attempt to resume work; that might fail again, but
assuming the firmware won't keep sending oversized requests forever, it should
eventually recover.

> 
> For comparison, the older Imagination host implementation also assumes a
> valid count, processes that many entries and echoes the count in the update:
> 
> https://android.googlesource.com/kernel/mediatek/+/android-mtk-3.18/drivers/staging/imgtec/rogue/rgxta3d.c#710

FYI this driver is very old (> 10 years). Newer versions of the consumer DDK
clamp the length of the response in RGXProcessRequestFreelistsReconstruction()
and additionally initialise each element of the clamped list to invalid,
ignoring anything above the limit.

> 
> The current ABI provides a per-freelist reconstruction-failed flag, but no
> whole-request error value for an invalid count.
> 
> What is the intended recovery when firmware supplies a count above the
> protocol maximum? Should the driver treat it as a firmware-fatal condition,
> complete a bounded subset, or use another firmware-defined response? Once
> that contract is clear, I can prepare a narrow patch without inventing new
> firmware semantics.
> 

In this case, (at least) clamping is fine for the reasons above.

Thanks,
Alessio