Re: [PATCH v2 03/10] drm/nouveau/disp: route GSP-RM display MMIO through nvkm_disp_func hooks

On Thu, Aug 20, 2026 at 9:01 PM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Pre-existing Use-After-Free and Double Free in `r535_disp_new` error path when `nvkm_disp_new_` fails after subdev registration.
> - [High] Pre-existing NULL pointer dereference in the error path of `r535_disp_new` due to unconditional lock initialization on a potentially NULL pointer.
> --
I saw those and can confirm they are very much real. However, fixing
them is out of scope here (I was actually going to roll them in at
first), because there are 5 (that I have seen, might be more)
r535_disp_ functions that have the exact same issues.

>
> --- Patch [3]: [PATCH v2 03/10] drm/nouveau/disp: route GSP-RM display MMIO through nvkm_disp_func hooks ---
> commit eaafa1eb740ad36427480951cde140701ddc6d3a
> Author: Mohamed Ahmed <[email protected]>
>
> drm/nouveau/disp: route GSP-RM display MMIO through nvkm_disp_func hooks
>
> The GSP-RM display code in rm/r535/disp.c borrows a few
> register-programming routines from engine/disp (the head-timing
> interrupt handler, vblank enables, armed head state and scanout position
> readback, the AVI/VSI infoframe writers and the GCP AVMute write) and so
> far picked them by name, which means it has to know which chip it runs
> on the moment a generation changes any of them.
>
> > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > index cd4451e625122..f3e55253bcbca 100644
> > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/disp.c
> [ ... ]
> > @@ -1688,6 +1729,7 @@ r535_disp_new(const struct nvkm_disp_func *hw, struct nvkm_device *device,
> >       rm->uevent = hw->uevent;
> >       rm->sor.cnt = r535_sor_cnt;
> >       rm->sor.new = r535_sor_new;
> > +     rm->gsp = hw->gsp;
> >       rm->ramht_size = hw->ramht_size;
> >
> >       rm->root.oclass = gpu->disp.class.root;
>
> [Severity: Critical]
> This is a pre-existing issue, but can the error path in r535_disp_new() lead
> to a use-after-free and double-free?
>
> If nvkm_disp_new_() fails after subdev registration, the subdevice is already
> attached to the device's subdev list. r535_disp_new() handles the error by
> freeing the dynamically allocated rm struct:
>
>         ret = nvkm_disp_new_(rm, device, type, inst, pdisp);
>         if (ret)
>                 kfree(rm);
>
> However, disp->func still points to this freed memory. Later, during device
> teardown on the error path, nvkm_subdev_del() invokes nvkm_disp_dtor() which
> evaluates the pointer:
>
>         if (disp->func && disp->func->dtor)
>                 disp->func->dtor(disp);
>
> This dereferences the freed rm struct and calls r535_disp_dtor(), which
> subsequently executes kfree(disp->func), causing a double-free.
>
> [Severity: High]
> This is another pre-existing issue, but can the same error path cause a NULL
> pointer dereference?
>
> If nvkm_disp_new_() fails early before allocating the struct, *pdisp is set
> to NULL. r535_disp_new() then unconditionally attempts to initialize a mutex
> on it:
>
>         ret = nvkm_disp_new_(rm, device, type, inst, pdisp);
>         if (ret)
>                 kfree(rm);
>
>         mutex_init(&(*pdisp)->super.mutex);
>         return ret;
>
> Passing an offset from NULL to mutex_init() will cause a panic when it tries
> to write to the mutex fields.
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3