This should probably be prefixed with the title "drm/nouveau/clk:", but I can
fix that before pushing it.
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
Will push it to the appropriate repository shortly
On Sun, 2022-03-27 at 15:58 +0800, Xiaomeng Tong wrote:
> The bug is here:
> if (nvkm_cstate_valid(clk, cstate, max_volt, clk->temp))
> return cstate;
>
> The list iterator value 'cstate' will *always* be set and non-NULL
> by list_for_each_entry_from_reverse(), so it is incorrect to assume
> that the iterator value will be unchanged if the list is empty or no
> element is found (In fact, it will be a bogus pointer to an invalid
> structure object containing the HEAD). Also it missed a NULL check
> at callsite and may lead to invalid memory access after that.
>
> To fix this bug, just return 'encoder' when found, otherwise return
> NULL. And add the NULL check.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 1f7f3d91ad38a ("drm/nouveau/clk: Respect voltage limits in
> nvkm_cstate_prog")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@xxxxxxxxx>
> ---
> drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> index 57199be082fd..c2b5cc5f97ed 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
> @@ -135,10 +135,10 @@ nvkm_cstate_find_best(struct nvkm_clk *clk, struct
> nvkm_pstate *pstate,
>
> list_for_each_entry_from_reverse(cstate, &pstate->list, head) {
> if (nvkm_cstate_valid(clk, cstate, max_volt, clk->temp))
> - break;
> + return cstate;
> }
>
> - return cstate;
> + return NULL;
> }
>
> static struct nvkm_cstate *
> @@ -169,6 +169,8 @@ nvkm_cstate_prog(struct nvkm_clk *clk, struct
> nvkm_pstate *pstate, int cstatei)
> if (!list_empty(&pstate->list)) {
> cstate = nvkm_cstate_get(clk, pstate, cstatei);
> cstate = nvkm_cstate_find_best(clk, pstate, cstate);
> + if (!cstate)
> + return -EINVAL;
> } else {
> cstate = &pstate->base;
> }
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
