Re: [PATCH v4 2/2] drm/nouveau/kms: guard NULL crtc in nv50_sor_atomic_disable()

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