Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The patch adds a guard against disable-without-enable or double-disable in `nv50_sor_atomic_disable()`, but misses an identical structural vulnerability in `nv50_msto_atomic_disable()`. -- --- Patch [2]: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable() --- commit 7ccc8c2fed1546597f7a2ed45ab47da4012b6d66 Author: Marek Czernohous <[email protected]> drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable() nv50_sor_atomic_disable() unconditionally computes nv50_head(nv_encoder->crtc) and dereferences the result a few lines later. nv_encoder->crtc is nouveau's own shadow pointer, set in .atomic_enable and cleared at the end of .atomic_disable. This patch restores the guard, as drm_WARN_ON_ONCE() instead of a silent return. > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 364227f5456f1..f532b0ed88809 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -1565,16 +1565,36 @@ static void > nv50_sor_atomic_disable(struct drm_encoder *encoder, struct > drm_atomic_commit *state) > { > struct nouveau_encoder *nv_encoder = nouveau_encoder(encoder); > - struct nv50_head *head = nv50_head(nv_encoder->crtc); > + struct nv50_head *head; > #ifdef CONFIG_DRM_NOUVEAU_BACKLIGHT [Severity: High] This isn't a bug introduced by this patch, but while fixing the disable-without-enable issue here, does a similar vulnerability exist in nv50_msto_atomic_disable() in the same file? Looking at nv50_msto_atomic_disable(): static void nv50_msto_atomic_disable(struct drm_encoder *encoder, struct drm_atomic_commit *state) { struct nv50_msto *msto = nv50_msto(encoder); struct nv50_mstc *mstc = msto->mstc; struct nv50_mstm *mstm = mstc->mstm; If this is called during session teardown without a matching .atomic_enable, couldn't msto->mstc be NULL, leading to a NULL pointer dereference when accessing mstc->mstm? > - struct nouveau_connector *nv_connector = > nv50_outp_get_old_connector(state, nv_encoder); > struct nouveau_drm *drm = nouveau_drm(nv_encoder->base.base.dev); > - struct nouveau_backlight *backlight = nv_connector->backlight; > - struct drm_dp_aux *aux = &nv_connector->aux; > int ret; > +#endif > > + /* nv_encoder->crtc is the driver's shadow pointer, set in > + * .atomic_enable (and by the boot-time hardware readback) and > + * cleared at the end of this function. NULL here > + * means disable-without-enable or a double disable; bail before > + * container_of() turns it into a bogus head pointer (checking the > + * result would not work, container_of(NULL) is never NULL). The > + * encoder release is handled by the commit_tail release loop, so > + * there is nothing to clean up here. > + */ > + if (drm_WARN_ON_ONCE(encoder->dev, !nv_encoder->crtc)) > + return; > + head = nv50_head(nv_encoder->crtc); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
