Nouveau - Mar 2022 - [PATCH] dispnv50: atom: fix an incorrect NULL check on list iterator

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 at vger.kernel.org
Fixes: 12885ecbfe62d ("drm/nouveau/kms/nvd9-: Add CRC support")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong at 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

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

Reviewed-by: Lyude Paul <lyude at 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 at vger.kernel.org
> Fixes: 12885ecbfe62d ("drm/nouveau/kms/nvd9-: Add CRC support")
> Signed-off-by: Xiaomeng Tong <xiam0nd.tong at 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