Re: [PATCH v5 1/5] drm/nouveau/gsp/r570: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] nouveau: memory corruption via unvalidated buffer write in nvkm_udevice_gcx_ready()
- [High] nouveau: NULL pointer dereference in nvkm_gsp_gcx_ready when GSP-RM is disabled
- [Medium] pm: autosuspend timer permanently disabled on GSP query failure
- [Low] style: mismatched object pointers used for RPC control allocation and deallocation
- [Low] build: dead declaration for r535_gsp_gcx_ready
--

commit 4cd99785097738ad0c1e828e75d9aae4edf01817
Author: Lyude Paul <lyude@redhat.com>
Subject: drm/nouveau/gsp/r570: Add support for INTERNAL_GCX_ENTRY_PREREQUISITE

This patch adds support for querying the OpenRM GSP for GC6/GCOFF readiness
before runtime suspending the GPU. If the hardware is not ready, the suspend
operation is delayed.

> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -1148,6 +1148,7 @@ nouveau_pmops_runtime_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
>  	struct nouveau_drm *drm = pci_get_drvdata(pdev);
> +	struct nvif_device *nvif = &drm->client.device;
>  	int ret;
>  
>  	if (!nouveau_pmops_runtime()) {
> @@ -1155,6 +1156,18 @@ nouveau_pmops_runtime_suspend(struct device *dev)
>  		return -EBUSY;
>  	}
>  
> +	// Check if the GPU itself is ready for runtime suspend, otherwise mark as busy and check
> +	// again in a bit.
> +	ret = nvif_device_gcx_ready(nvif);
> +	if (ret < 0) {
> +		NV_ERROR(drm, "Failed to query GCX readiness (returned %d)\n", ret);
> +		return -EBUSY;

[Severity: Medium]
Will this error path leave the PM core's autosuspend loop permanently disabled?

If nvif_device_gcx_ready() fails and returns -EBUSY without calling
pm_runtime_mark_last_busy(), the PM core won't re-arm the autosuspend timer
because the calculated expiration is in the past. Should this failure path
also mark the device as busy so it retries, or is disabling autosuspend
intentional here?

> +	} else if (!(ret & NV_DEVICE_GCOFF_READY)) {
> +		NV_DEBUG(drm, "GPU isn't ready for suspend yet, delaying...\n");
> +		pm_runtime_mark_last_busy(dev);
> +		return -EBUSY;
> +	}
>  
>  	nouveau_switcheroo_optimus_dsm();
>  	ret = nouveau_do_suspend(drm, true);

> [ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> --- a/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/engine/device/user.c
> @@ -189,6 +190,38 @@ 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;
> +
> +	if (!gsp) {
> +		args->v0.ready = NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY;
> +		return 0;
> +	}

[Severity: High]
Could this result in an out-of-bounds write if the caller provides an empty
or undersized buffer?

The ioctl handler allocates exactly the requested size, but if gsp is NULL
(e.g., NvGspRm=0), this writes to args->v0.ready before nvif_unpack() is
called to validate the size. A crafted 0-byte payload for this method could
cause a write past the allocation into kernel heap memory. Should the
size validation happen before any writes to the arguments?

> +
> +	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/base.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/base.c
> @@ -47,6 +48,15 @@ nvkm_gsp_intr_stall(struct nvkm_gsp *gsp, enum nvkm_subdev_type type, int inst)
>  	return -ENOENT;
>  }
>  
> +int
> +nvkm_gsp_gcx_ready(struct nvkm_gsp *gsp)
> +{
> +	if (!gsp->rm->api->gsp->gcx_ready)

[Severity: High]
Does this code crash with a NULL pointer dereference when GSP is present but
RM is disabled?

When RM is disabled (e.g., via NvGspRm=0 or firmware fallback to
gv100_gsp_nofw), device->gsp is allocated but gsp->rm is left as NULL.
Since nouveau_pmops_runtime_suspend() calls this unconditionally when
runtime PM is active, this looks like it will panic when dereferencing
gsp->rm->api. Should there be a check for !gsp->rm here?

> +		return NV_DEVICE_GC6_READY | NV_DEVICE_GCOFF_READY;
> +
> +	return gsp->rm->api->gsp->gcx_ready(gsp);
> +}

> [ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/priv.h
> @@ -93,6 +93,7 @@ void r535_gsp_dtor(struct nvkm_gsp *);
>  int r535_gsp_oneinit(struct nvkm_gsp *);
>  int r535_gsp_init(struct nvkm_gsp *);
>  int r535_gsp_fini(struct nvkm_gsp *, enum nvkm_suspend_state suspend);
> +int r535_gsp_gcx_ready(struct nvkm_gsp *gsp);

[Severity: Low]
This isn't a bug, but this declaration appears to be dead code, as the
implementation for r535_gsp_gcx_ready was removed in earlier revisions
of this patch series.

>  
>  int nvkm_gsp_new_(const struct nvkm_gsp_fwif *, struct nvkm_device *, enum nvkm_subdev_type, int,
>  		  struct nvkm_gsp **);

> [ ... ]

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/gsp.c
> @@ -215,6 +216,32 @@ r570_gsp_set_rmargs(struct nvkm_gsp *gsp, bool resume)
>  	args->bDmemStack = 1;
>  }
>  
> +int
> +r570_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));
> +	if (IS_ERR(ctrl))
> +		return PTR_ERR(ctrl);
> +
> +	if (ctrl->bIsGC6Satisfied)
> +		ret |= NV_DEVICE_GC6_READY;
> +	if (ctrl->bIsGCOFFSatisfied)
> +		ret |= NV_DEVICE_GCOFF_READY;
> +
> +	nvkm_debug(&gsp->subdev,
> +		   "GCX ready status: GC6=%s GCOFF=%s\n",
> +		   str_yes_no(ctrl->bIsGC6Satisfied), str_yes_no(ctrl->bIsGCOFFSatisfied));
> +
> +	nvkm_gsp_rm_ctrl_done(&gsp->internal.client.object, ctrl);

[Severity: Low]
Is this using the intended object pointer to finalize the control buffer?

The allocation is done using &gsp->internal.device.subdevice, but here it is
finalized with &gsp->internal.client.object. Since both resolve to the same
underlying GSP pointer for the RPC completion it likely works, but shouldn't it
use the same object for both allocation and deallocation?

> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917170042.440276-1-lyude@redhat.com?part=1