[PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[Xiaomeng Tong]

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(-)

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]);
-- 
2.17.1

Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[Emil Velikov]

On Sun, 27 Mar 2022 at 08:39, Xiaomeng Tong <xiam0nd.tong@gmail.com> 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.
>

Isn't this covered by the upcoming list* iterator rework [1] or is
this another iterator glitch?
IMHO we should be looking at fixing the implementation and not the
hundreds of users through the kernel.

HTH
-Emil
[1] https://lwn.net/Articles/887097/

Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[Xiaomeng Tong]

on Sun, 27 Mar 2022 16:59:28 +0100, Emil Velikov wrote:
> On Sun, 27 Mar 2022 at 08:39, Xiaomeng Tong <xiam0nd.tong@gmail.com> 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.
> >
> 
> Isn't this covered by the upcoming list* iterator rework [1] or is
> this another iterator glitch?

Actually, it is a part of the upcoming work.

> IMHO we should be looking at fixing the implementation and not the
> hundreds of users through the kernel.
>
> HTH
> -Emil
> [1] https://lwn.net/Articles/887097/

Yes, you are right. This has also been taken into account by the upcoming
list iterator rework to avoid a lot uesr' changes as much as possible.

However, this patch is fixing a potential bug caused by incorrect use of
list iterator outside the loop, which can not be fixed by the implementation
itself.

--
Xiaomeng Tong

Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[Lyude Paul]

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

Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

[Emil Velikov]

On Mon, 28 Mar 2022 at 03:09, Xiaomeng Tong <xiam0nd.tong@gmail.com> wrote:
>
> on Sun, 27 Mar 2022 16:59:28 +0100, Emil Velikov wrote:
> > On Sun, 27 Mar 2022 at 08:39, Xiaomeng Tong <xiam0nd.tong@gmail.com> 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.
> > >
> >
> > Isn't this covered by the upcoming list* iterator rework [1] or is
> > this another iterator glitch?
>
> Actually, it is a part of the upcoming work.
>
> > IMHO we should be looking at fixing the implementation and not the
> > hundreds of users through the kernel.
> >
> > HTH
> > -Emil
> > [1] https://lwn.net/Articles/887097/
>
> Yes, you are right. This has also been taken into account by the upcoming
> list iterator rework to avoid a lot uesr' changes as much as possible.
>
> However, this patch is fixing a potential bug caused by incorrect use of
> list iterator outside the loop, which can not be fixed by the implementation
> itself.
>

I see, thanks for the information o/

-Emil