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 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Integer underflow in `r535_gsp_msgq_recv_one_elem` due to missing lower bounds check on `rpc->length` causes massive out-of-bounds memory copy.
- [Critical] A Time-of-Check to Time-of-Use (TOCTOU) double-fetch vulnerability in `r535_gsp_msgq_recv` allows a malicious GSP firmware to cause a kernel heap buffer overflow.
- [High] NULL pointer dereference when `r535_gsp_msgq_recv()` encounters an invalid RPC message size.
- [High] Out-of-bounds memory reads due to unvalidated array counts from firmware in `r535_gsp_intr_get_table` and `r535_gsp_get_static_info_fb`.
--

commit 88b24214ef8688e688f55f1aef5cd0f776cdce80
Author: Hongling Zeng <zenghongling@kylinos.cn>
Subject: nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd()

This commit cleans up the error checking in nvkm_gsp_rpc_rd(). Since
nvkm_gsp_rpc_get() returns only valid pointers or error pointers, the
code is updated to use IS_ERR() instead of IS_ERR_OR_NULL() to match the
actual API 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: High]
This isn't a bug introduced by this patch, but if r535_gsp_msgq_recv() 
returns NULL when rpc->length exceeds max_rpc_size, doesn't this bubble up 
through nvkm_gsp_rpc_push() and nvkm_gsp_rpc_rd()? 

If callers like r535_gsp_get_static_info() only check IS_ERR(), won't they 
dereference NULL? 

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;
    ...
}

Should r535_gsp_msgq_recv() return an error pointer like ERR_PTR(-EIO) 
instead of NULL to match this API contract?

[Severity: High]
This is a pre-existing issue, but does r535_gsp_intr_get_table() properly 
bound its iteration over ctrl->tableLen? 

If a faulty firmware provides a count larger than 
NV2080_CTRL_INTERNAL_INTR_MAX_TABLE_SIZE, won't this loop read out-of-bounds 
into adjacent memory? 

Similarly, does r535_gsp_get_static_info_fb() need to verify 
info->numFBRegions against the fbRegion array bounds before iterating?

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing lower 
bounds check for rpc->length in r535_gsp_msgq_recv()? 

If rpc->length is 0, len evaluates to 0 for continuation records in 
r535_gsp_msgq_recv_one_elem(). Doesn't len - sizeof(struct nvfw_gsp_rpc) 
then underflow, causing a massive out-of-bounds memory copy?

drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv_one_elem() {
    ...
	if (info->continuation)
		memcpy(buf, mqe->data + sizeof(struct nvfw_gsp_rpc),
		       len - sizeof(struct nvfw_gsp_rpc));
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does r535_gsp_msgq_recv() safely handle 
rpc->length from shared memory? 

It appears the firmware-provided rpc->length is read directly from mqe->data 
three times. If a malicious firmware increases this value after the bounds 
check and allocation, but before info.gsp_rpc_len = rpc->length, couldn't 
r535_gsp_msgq_recv_one_elem() cause a heap overflow when copying up to 
info.gsp_rpc_len bytes?

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