Re: [PATCH v5 5/5] nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] NULL pointer dereference when GSP firmware sends a malformed message with invalid length.
- [Medium] Memory leak of large RPC request buffers on error paths.
--

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

nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd()

This patch changes nvkm_gsp_rpc_rd() to check IS_ERR() instead of
IS_ERR_OR_NULL() for the return value of nvkm_gsp_rpc_get(). It also
notes that nvkm_gsp_rpc_push() should return error pointers, not NULL,
when using the RECV policy.

> Note: nvkm_gsp_rpc_push() can return NULL when policy is NOWAIT or
> NOSEQ, but nvkm_gsp_rpc_rd() uses RECV policy which expects a reply.
> If the RPC fails to get a reply, it should return an error pointer,
> not NULL.
>
> Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>

[Severity: High]
This is a pre-existing issue, but is the assumption in the commit message
accurate?

Looking at r535_gsp_msgq_recv() in rpc.c, if a malformed message is
received, it explicitly returns NULL:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv() {
	if (WARN_ON(rpc->length > max_rpc_size))
		return NULL;
	...
}

This NULL propagates through the reply handlers to nvkm_gsp_rpc_push(),
which returns it to the caller.

Because callers like r535_gsp_get_static_info() only check IS_ERR(),
this leads to a NULL pointer dereference:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c:r535_gsp_get_static_info() {
	if (IS_ERR(rpc))
		return PTR_ERR(rpc);

	gsp->internal.client.object.client = &gsp->internal.client;
	...
}

Other callers like r535_gsp_rpc_rm_ctrl_push() implicitly convert this NULL
return to 0 (success) via PTR_ERR(rpc), masking the failure.

Should the underlying r535_gsp_msgq_recv() be modified to return an
ERR_PTR to match the expected contract?

> diff --git a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> index 64fed208e4cf8..a061779e0107b 100644
> --- a/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> +++ b/drivers/gpu/drm/nouveau/include/nvkm/subdev/gsp.h
> @@ -293,7 +293,7 @@ nvkm_gsp_rpc_rd(struct nvkm_gsp *gsp, u32 fn, u32 argc)
>  {
>  	void *argv = nvkm_gsp_rpc_get(gsp, fn, argc);
>  
> -	if (IS_ERR_OR_NULL(argv))
> +	if (IS_ERR(argv))
>  		return argv;
>  
>  	return nvkm_gsp_rpc_push(gsp, argv, NVKM_GSP_RPC_REPLY_RECV, argc);
        ^^^^

[Severity: Medium]
This is a pre-existing issue, but does r535_gsp_rpc_push() leak the msg
buffer on error paths?

Looking at r535_gsp_rpc_push() in rpc.c, when payload_size > max_payload_size,
the large msg buffer is freed only on success:

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() {
	...
done:
	if (!IS_ERR(repv))
		kvfree(msg);

	return repv;
}

If any intermediate chunk allocation fails, or if the final reply results
in an error pointer, could this leak the large RPC request buffers?

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