Re: [PATCH v4 1/5] nouveau/gsp/rpc: Document RPC function return value contracts

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Returning NULL on an invalid RPC message length deadlocks the message queue and causes downstream wrappers to treat errors as success, leading to NULL pointer dereferences.
- [High] Lack of bounds checking on continuation chunks allows a malformed or malicious GSP message to cause integer underflows and heap buffer overflows.
- [High] The original split RPC message buffer is leaked on error paths when the payload exceeds max_payload_size.
--

commit b2d404f83fcf801450f3eade502dc33e20d3fbe6
Author: Hongling Zeng <zenghongling@kylinos.cn>
Subject: nouveau/gsp/rpc: Document RPC function return value contracts

This commit adds kernel-doc comments to document the return value semantics
of RPC functions in the Nouveau GSP RM r535/rpc.c file.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c
> index 3ca3de8f43408..2ca6de4094e63 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c

[ ... ]

> @@ -283,6 +300,15 @@ r535_gsp_msgq_recv_one_elem(struct nvkm_gsp *gsp,
>  	return buf;
>  }
>  
> +/**
> + * r535_gsp_msgq_recv - Receive RPC message(s) from GSP queue
> + * @gsp: GSP device
> + * @gsp_rpc_len: Expected RPC length
> + * @retries: Retry counter
> + *
> + * Return: Pointer to received buffer on success, ERR_PTR() on error,
> + *         or NULL if RPC length is invalid.
> + */
>  static void *
>  r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries)
>  {

[Severity: High]
This is a pre-existing issue, but does returning NULL here when the RPC
length is invalid cause the message queue to deadlock and trigger NULL
pointer dereferences downstream?

Looking at r535_gsp_msgq_recv(), it returns NULL without advancing the read
pointer:

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

Several downstream functions seem to mishandle this NULL return. For
example, in r535_gsp_rpc_rm_ctrl_push():

	if (IS_ERR_OR_NULL(rpc)) {
		*params = NULL;
		return PTR_ERR(rpc);
	}

This converts IS_ERR_OR_NULL(NULL) to PTR_ERR(NULL) == 0, signaling
success to the caller while setting *params to NULL.

Similarly, nvkm_gsp_rm_alloc_wr() checks IS_ERR(repv) and returns success
since IS_ERR is false for NULL. Callers like r535_gsp_get_static_info()
then check the reply with IS_ERR() instead of IS_ERR_OR_NULL(), leading
directly to a NULL pointer dereference.


[Severity: High]
This is also a pre-existing issue in r535_gsp_msgq_recv(), but can a
malformed GSP message cause integer underflows and heap overflows during
continuation chunk processing?

In the continuation loop:

	info.gsp_rpc_len = rpc->length;
	info.continuation = true;

	rpc = r535_gsp_msgq_recv_one_elem(gsp, &info);
	...
	size = info.gsp_rpc_len - sizeof(*rpc);
	expected -= size;

If rpc->length is less than sizeof(*rpc), the subtraction for size will
underflow to a massive value, which then appears to cause an out-of-bounds
memcpy in r535_gsp_msgq_recv_one_elem(). Is there a missing bounds check
against the remaining expected capacity and the minimum header size?

[ ... ]

> @@ -628,6 +704,17 @@ r535_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 payload_size)
>  	return rpc->data;
>  }
>  
> +/**
> + * r535_gsp_rpc_push - Push RPC message to GSP and wait for reply
> + * @gsp: GSP device
> + * @payload: RPC payload to send
> + * @policy: Reply policy (NOWAIT, NOSEQ, RECV, or POLL)
> + * @gsp_rpc_len: Expected RPC length in the reply
> + *
> + * Return: NULL when policy is NOWAIT/NOSEQ (no reply expected) or
> + *         POLL (successful message processing), pointer to reply data
> + *         on success, or ERR_PTR() on error.
> + */
>  static void *
>  r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload,
>  		  enum nvkm_gsp_rpc_reply_policy policy, u32 gsp_rpc_len)

[Severity: High]
This is a pre-existing issue, but does this function leak the original
split RPC message buffer on error paths?

In r535_gsp_rpc_push(), when the payload exceeds max_payload_size, it is
chunked and sent iteratively. If an intermediate chunk fails to send, the
code jumps to the done label:

			repv = r535_gsp_rpc_send(gsp, next, NVKM_GSP_RPC_REPLY_NOWAIT, 0);
			if (IS_ERR(repv))
				goto done;
...
		if (!IS_ERR(repv))
			kvfree(msg);

done:
	mutex_unlock(&gsp->cmdq.mutex);
	return repv;

Because kvfree(msg) is only executed if the operations are successful
(!IS_ERR(repv)), does the original msg buffer remain permanently leaked
when an error occurs?

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