| Message ID | 20220327073925.11121-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dispnv50: atom: fix an incorrect NULL check on list iterator | expand |
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/
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
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]);
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
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]);
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(-)