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. -- --- 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
