Re: [PATCH v5 4/5] nouveau/gsp/rm/bar: Cleanup IS_ERR_OR_NULL usage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] `r535_gsp_msgq_recv()` incorrectly returns `NULL` instead of an `ERR_PTR` when receiving a message exceeding `max_rpc_size`, leading to NULL pointer dereferences in upstream callers.
- [Medium] In `r535_gsp_rpc_push()`, when a large RPC payload encounters an error during transmission or waiting for a reply, the original message buffer is leaked.
--

commit c28b7cbd3dab1fd32a31ef4df86d6f6aaf9ad40b
Author: Hongling Zeng <zenghongling@kylinos.cn>

nouveau/gsp/rm/bar: Cleanup IS_ERR_OR_NULL usage

This commit updates r535_bar_bar2_update_pde() to use IS_ERR() instead of
IS_ERR_OR_NULL() when checking the return value of nvkm_gsp_rpc_get(). This
aligns with the function's contract of never returning NULL.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c
> index fae08ac3b18c7..9cd68f8622d36 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/bar.c
> @@ -55,7 +55,7 @@ r535_bar_bar2_update_pde(struct nvkm_gsp *gsp, u8 page_shift, u64 pdbe)
>  	rpc_update_bar_pde_v15_00 *rpc;
>  
>  	rpc = nvkm_gsp_rpc_get(gsp, NV_VGPU_MSG_FUNCTION_UPDATE_BAR_PDE, sizeof(*rpc));
> -	if (WARN_ON(IS_ERR_OR_NULL(rpc)))
> +	if (WARN_ON(IS_ERR(rpc)))
>  		return -EIO;
>  
>  	rpc->info.barType = NV_RPC_UPDATE_PDE_BAR_2;

[Severity: High]
This is a pre-existing issue, but there is a case where the GSP RPC
infrastructure incorrectly returns a NULL pointer. If the message queue
receives an unexpectedly large message:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv() {
	mqe = r535_gsp_msgq_get_entry(gsp);
	rpc = (struct nvfw_gsp_rpc *)mqe->data;

	if (WARN_ON(rpc->length > max_rpc_size))
		return NULL;
...
}

This NULL return value propagates up through r535_gsp_msg_recv() and
r535_gsp_rpc_handle_reply(), causing nvkm_gsp_rpc_rd() to return NULL.

Does this lead to a NULL pointer dereference in callers like
r535_gsp_get_static_info() since they only validate the result with IS_ERR()?

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c:r535_gsp_get_static_info() {
	rpc = nvkm_gsp_rpc_rd(gsp, NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO, sizeof(*rpc));
	if (IS_ERR(rpc))
		return PTR_ERR(rpc);

	gsp->internal.client.object.client = &gsp->internal.client;
...
	gsp->internal.client.object.handle = rpc->hInternalClient;
...
}

[Severity: Medium]
This is also a pre-existing issue, but does the slow path in
r535_gsp_rpc_push() leak the original message buffer when an error occurs?

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() {
...
	if (payload_size > max_payload_size) {
...
		/* Send initial RPC. */
		next = r535_gsp_rpc_get(gsp, fn, max_payload_size);
		if (IS_ERR(next)) {
			repv = next;
			goto done;
		}
...
		/* Wait for reply. */
		repv = r535_gsp_rpc_handle_reply(gsp, fn, policy, payload_size +
						 sizeof(*rpc));
		if (!IS_ERR(repv))
			kvfree(msg);
	} else {
...
done:
	mutex_unlock(&gsp->cmdq.mutex);
	return repv;
}

In the fast path, r535_gsp_cmdq_push() takes ownership and unconditionally
frees the buffer. However, in the chunked sending path above, if there is an
error getting a chunk (IS_ERR(next)) or receiving a reply (IS_ERR(repv)),
execution jumps to the done label without calling kvfree(msg). Could this
permanently leak the large payload buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260622081136.61695-1-zenghongling@kylinos.cn?part=4