Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <30057caf791dd789fe715715d1c1973994a91953.camel@redhat.com>
Subject: Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list
 iterator
From:   Lyude Paul <lyude@redhat.com>
To:     Xiaomeng Tong <xiam0nd.tong@gmail.com>, bskeggs@redhat.com,
        kherbst@redhat.com, airlied@linux.ie, daniel@ffwll.ch
Cc:     yangyingliang@huawei.com, contact@emersion.fr, airlied@gmail.com,
        dri-devel@lists.freedesktop.org, nouveau@lists.freedesktop.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org
Date:   Mon, 28 Mar 2022 17:20:40 -0400
In-Reply-To: <20220327073925.11121-1-xiam0nd.tong@gmail.com>
References: <20220327073925.11121-1-xiam0nd.tong@gmail.com>
Organization: Red Hat Inc.
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.DarkModeFix.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Reviewed-by: Lyude Paul <lyude@redhat.com>

Will push this to the appropriate repository shortly.

On Sun, 2022-03-27 at 15:39 +0800, Xiaomeng Tong wrote:
> The bug is here:
>         return encoder;
> 
> The list iterator value 'encoder' will *always* be set and non-NULL
> by drm_for_each_encoder_mask(), so it is incorrect to assume that the
> iterator value will be NULL if the list is empty or no element found.
> Otherwise it will bypass some NULL checks and lead to invalid memory
> access passing the check.
> 
> To fix this bug, just return 'encoder' when found, otherwise return
> NULL.
> 
> Cc: stable@vger.kernel.org
> Fixes: 12885ecbfe62d ("drm/nouveau/kms/nvd9-: Add CRC support")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong@gmail.com>
> ---
>  drivers/gpu/drm/nouveau/dispnv50/atom.h |  6 +++---
>  drivers/gpu/drm/nouveau/dispnv50/crc.c  | 27 ++++++++++++++++++++-----
>  2 files changed, 25 insertions(+), 8 deletions(-)
> (also 
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/atom.h
> b/drivers/gpu/drm/nouveau/dispnv50/atom.h
> index 3d82b3c67dec..93f8f4f64578 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/atom.h
> +++ b/drivers/gpu/drm/nouveau/dispnv50/atom.h
> @@ -160,14 +160,14 @@ nv50_head_atom_get(struct drm_atomic_state *state,
> struct drm_crtc *crtc)
>  static inline struct drm_encoder *
>  nv50_head_atom_get_encoder(struct nv50_head_atom *atom)
>  {
> -       struct drm_encoder *encoder = NULL;
> +       struct drm_encoder *encoder;
>  
>         /* We only ever have a single encoder */
>         drm_for_each_encoder_mask(encoder, atom->state.crtc->dev,
>                                   atom->state.encoder_mask)
> -               break;
> +               return encoder;
>  
> -       return encoder;
> +       return NULL;
>  }
>  
>  #define nv50_wndw_atom(p) container_of((p), struct nv50_wndw_atom, state)
> diff --git a/drivers/gpu/drm/nouveau/dispnv50/crc.c
> b/drivers/gpu/drm/nouveau/dispnv50/crc.c
> index 29428e770f14..b834e8a9ae77 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/crc.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/crc.c
> @@ -390,9 +390,18 @@ void nv50_crc_atomic_check_outp(struct nv50_atom *atom)
>                 struct nv50_head_atom *armh =
> nv50_head_atom(old_crtc_state);
>                 struct nv50_head_atom *asyh =
> nv50_head_atom(new_crtc_state);
>                 struct nv50_outp_atom *outp_atom;
> -               struct nouveau_encoder *outp =
> -                       nv50_real_outp(nv50_head_atom_get_encoder(armh));
> -               struct drm_encoder *encoder = &outp->base.base;
> +               struct nouveau_encoder *outp;
> +               struct drm_encoder *encoder, *enc;
> +
> +               enc = nv50_head_atom_get_encoder(armh);
> +               if (!enc)
> +                       continue;
> +
> +               outp = nv50_real_outp(enc);
> +               if (!outp)
> +                       continue;
> +
> +               encoder = &outp->base.base;
>  
>                 if (!asyh->clr.crc)
>                         continue;
> @@ -443,8 +452,16 @@ void nv50_crc_atomic_set(struct nv50_head *head,
>         struct drm_device *dev = crtc->dev;
>         struct nv50_crc *crc = &head->crc;
>         const struct nv50_crc_func *func = nv50_disp(dev)->core->func->crc;
> -       struct nouveau_encoder *outp =
> -               nv50_real_outp(nv50_head_atom_get_encoder(asyh));
> +       struct nouveau_encoder *outp;
> +       struct drm_encoder *encoder;
> +
> +       encoder = nv50_head_atom_get_encoder(asyh);
> +       if (!encoder)
> +               return;
> +
> +       outp = nv50_real_outp(encoder);
> +       if (!outp)
> +               return;
>  
>         func->set_src(head, outp->or, nv50_crc_source_type(outp, asyh-
> >crc.src),
>                       &crc->ctx[crc->ctx_idx]);

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat