Re: [PATCH v2 3/4] drm/nouveau/device: don't use the pstate cursor after the loop

[lyude]

Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>

On Sun, 2026-07-12 at 14:36 +0200, Francesco Magazzu wrote:
> nvkm_control_mthd_pstate_attr() looks up the pstate at the index
> supplied
> by userspace by walking clk->states, and then keeps using the
> list_for_each_entry cursor after the loop.  This is not triggerable
> today:
> the function already rejects args->v0.state >= clk->state_nr before
> the
> loop, and clk->state_nr is kept in sync with the number of entries on
> clk->states, so the lookup always breaks on a real entry.
> 
> Should the loop ever run to completion, the cursor would point at the
> list
> head rather than at a pstate, and the pstate->base.domain[] read and
> the
> walk of pstate->list that follow would read past it.  Rather than
> leave
> that trap in place, track whether the entry was found and return -
> EINVAL if
> it was not, like the other lookup failures in this function.
> 
> No functional change.
> 
> Signed-off-by: Francesco Magazzu <postadelmaga@xxxxxxxxx>
> ---
>  drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> index f2e9a0626..28702741a 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/ctrl.c
> @@ -74,6 +74,7 @@ nvkm_control_mthd_pstate_attr(struct nvkm_control
> *ctrl, void *data, u32 size)
>  	const struct nvkm_domain *domain;
>  	struct nvkm_pstate *pstate;
>  	struct nvkm_cstate *cstate;
> +	bool found = false;
>  	int i = 0, j = -1;
>  	u32 lo, hi;
>  	int ret = -ENOSYS;
> @@ -104,10 +105,15 @@ nvkm_control_mthd_pstate_attr(struct
> nvkm_control *ctrl, void *data, u32 size)
>  
>  	if (args->v0.state !=
> NVIF_CONTROL_PSTATE_ATTR_V0_STATE_CURRENT) {
>  		list_for_each_entry(pstate, &clk->states, head) {
> -			if (i++ == args->v0.state)
> +			if (i++ == args->v0.state) {
> +				found = true;
>  				break;
> +			}
>  		}
>  
> +		if (!found)
> +			return -EINVAL;
> +
>  		lo = pstate->base.domain[domain->name];
>  		hi = lo;
>  		list_for_each_entry(cstate, &pstate->list, head) {