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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <30057caf791dd789fe715715d1c1973994a91953.camel@redhat.com>
Date:   Mon, 28 Mar 2022 17:20:40 -0400
From:   Lyude Paul <lyude@...hat.com>
To:     Xiaomeng Tong <xiam0nd.tong@...il.com>, bskeggs@...hat.com,
        kherbst@...hat.com, airlied@...ux.ie, daniel@...ll.ch
Cc:     yangyingliang@...wei.com, contact@...rsion.fr, airlied@...il.com,
        dri-devel@...ts.freedesktop.org, nouveau@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] dispnv50: atom: fix an incorrect NULL check on list
 iterator

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