Re: [PATCH] clk: base: fix an incorrect NULL check on list iterator

[Lyude Paul]

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