Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] drm/nouveau: OOB write in nvkm_udevice_gcx_ready() on non-GSP devices - [High] drm/nouveau: NULL pointer dereference in r535_gsp_gcx_ready() without GSP RM --
commit 3c9696115b3fc14a9b7b98dbb2d52910189ce9c7 Author: Lyude Paul <[email protected]> drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE This commit implements the NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE command to query GSP about whether the GPU is ready to enter Gc6 and/or GcOff states. It updates nouveau_pmops_runtime_suspend() to consult this readiness state before suspending. > diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > index 23d11d8221cb6..c47d6daa9e5aa 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c > +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c [ ... ] > @@ -189,6 +191,41 @@ nvkm_udevice_time(struct nvkm_udevice *udev, void *data, > u32 size) > return ret; > } > > +static int > +nvkm_udevice_gcx_ready(struct nvkm_udevice *udev, void *data, u32 size) > +{ > + struct nvkm_object *object = &udev->object; > + struct nvkm_device *device = udev->device; > + struct nvkm_gsp *gsp = device->gsp; > + union { > + struct nv_device_gcx_ready_v0 v0; > + } *args = data; > + int ret = -ENOSYS; > + > + /* XXX: We don't know any way of performing this check outside of GSP, > so just assume > + * things are ready for non-GSP clients. > + */ > + if (!gsp) { > + args->v0.ready = NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY; [Severity: High] Does this code write out of bounds? In nvkm_udevice_gcx_ready(), args->v0.ready is written here before nvif_unpack() validates the payload size. If a userspace process issues this ioctl with a size of 0, the data pointer would point past the end of the allocated header. Writing to args->v0.ready would then cause a 1-byte heap out-of-bounds write. [Severity: High] Can this condition lead to a NULL pointer dereference? This checks if GSP exists (!gsp), but doesn't verify if OpenRM is actually in use via nvkm_gsp_rm(gsp). If GSP firmware is instantiated but RM is disabled, execution will fall through to call r535_gsp_gcx_ready(), which assumes the RM control structures are fully initialized. > + return 0; > + } > + > + nvif_ioctl(object, "device gcx ready size %d\n", size); > + ret = nvif_unpack(ret, &data, &size, args->v0, 0, 0, false); [ ... ] > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c > index 94925f1590ea4..09b17b85dc1ac 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c [ ... ] > @@ -1721,6 +1722,31 @@ r535_gsp_sr_data_size(struct nvkm_gsp *gsp) > return meta->gspFwWprEnd - meta->gspFwWprStart; > } > > +int > +r535_gsp_gcx_ready(struct nvkm_gsp *gsp) > +{ > + NV2080_CTRL_INTERNAL_GCX_ENTRY_PREREQUISITE_PARAMS *ctrl; > + int ret = 0; > + > + ctrl = nvkm_gsp_rm_ctrl_rd(&gsp->internal.device.subdevice, > + > NV2080_CTRL_CMD_INTERNAL_GCX_ENTRY_PREREQUISITE, > + sizeof(*ctrl)); [Severity: High] Does this call dereference an uninitialized pointer when GSP RM is absent? If nvkm_udevice_gcx_ready() calls this function on a device that has GSP present but OpenRM disabled, the internal GSP RM control structures are not initialized. Calling nvkm_gsp_rm_ctrl_rd() here will dereference the NULL client pointer, leading to a kernel panic. > + if (IS_ERR(ctrl)) > + return PTR_ERR(ctrl); > + > + if (ctrl->bIsGC6Satisfied) > + ret |= NV_DEVICE_GC6_READY; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
