Re: [PATCH v4 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] Memory leak of the RPC message buffer in r535_gsp_rpc_push() when an error occurs while sending large payloads.
- [High] Malformed GSP firmware messages cause a NULL pointer dereference via nvkm_gsp_rm_ctrl_rd().
--

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

nouveau/gsp/rm/bar: Cleanup IS_ERR_OR_NULL usage

This commit cleans up the usage of IS_ERR_OR_NULL in
r535_bar_bar2_update_pde() by using IS_ERR() instead, matching the
actual return value contract of nvkm_gsp_rpc_get() which never returns
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 isn't a bug introduced by this patch, but while reviewing this cleanup,
I noticed another IS_ERR_OR_NULL usage in r535_gsp_rpc_rm_ctrl_push() that
causes a NULL pointer dereference.

When a malformed GSP firmware message is received:

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 to r535_gsp_rpc_rm_ctrl_push():

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/ctrl.c:r535_gsp_rpc_rm_ctrl_push() {
    ...
	rpc = nvkm_gsp_rpc_push(gsp, rpc, NVKM_GSP_RPC_REPLY_RECV, repc);
	if (IS_ERR_OR_NULL(rpc)) {
		*params = NULL;
		return PTR_ERR(rpc);
	}
    ...
}

Does PTR_ERR(rpc) evaluate to 0 here when rpc is NULL? If so, this erroneously
signals success to callers like nvkm_gsp_rm_ctrl_rd(), which will then
blindly dereference the NULL pointer. Could this be handled differently so
an error code is returned instead?

[Severity: High]
This isn't a bug introduced by this patch, but looking at r535_gsp_rpc_push(),
could there be a memory leak of the RPC message buffer when an error occurs
while sending large payloads?

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() {
    ...
	if (payload_size > max_payload_size) {
    ...
		while (remain_payload_size) {
			u32 size = min(remain_payload_size,
				       max_payload_size);

			next = r535_gsp_rpc_get(gsp, NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD, size);
			if (IS_ERR(next)) {
				repv = next;
				goto done;
			}
    ...
		if (!IS_ERR(repv))
			kvfree(msg);
	} else {
    ...
done:
	mutex_unlock(&gsp->cmdq.mutex);
	return repv;
}

When IS_ERR(next) is true during chunking, the execution jumps to the done
label, bypassing the kvfree(msg) cleanup logic. The caller function then sets
its pointer to NULL on error, losing the reference to the original msg
buffer. Does this result in a resource leak on this error path?

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