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