Re: [PATCH v3 1/5] drm/nouveau/gsp/r535: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

[sashiko-bot@kernel.org]

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 <lyude@redhat.com>

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/20260916001603.22394-1-lyude@redhat.com?part=1