Re: [PATCH v3 2/5] nouveau/gsp/rpc: Cleanup incorrect IS_ERR_OR_NULL in rpc.c

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing validation of `rpc->length` from untrusted GSP firmware leads to integer underflows and massive out-of-bounds memory accesses.
- [High] Incorrect destination offset when a continuation message wraps around the ring buffer leads to payload corruption and an out-of-bounds write.
- [High] `r535_gsp_msgq_recv()` returns `NULL` on oversized RPC messages, leading to NULL pointer dereferences in callers.
--

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

nouveau/gsp/rpc: Cleanup incorrect IS_ERR_OR_NULL in rpc.c
    
Clean up incorrect IS_ERR_OR_NULL() usage for functions that never
return NULL:
- r535_gsp_msgq_peek() never returns NULL
- r535_gsp_msgq_recv_one_elem() never returns NULL
    
These functions should be checked with IS_ERR() instead.

> 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 35272c1538699..25d3adbc13550 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

[ ... ]

> @@ -359,7 +359,7 @@ r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries)
>  		info.continuation = true;
>  
>  		rpc = r535_gsp_msgq_recv_one_elem(gsp, &info);
> -		if (IS_ERR_OR_NULL(rpc)) {
> +		if (IS_ERR(rpc)) {
>  			kvfree(buf);
>  			return rpc;
>  		}

[Severity: Critical]
This is a pre-existing issue, but does r535_gsp_msgq_recv() validate the
size of rpc->length coming from the untrusted GSP firmware?

If a compromised GSP sends an rpc->length less than sizeof(*rpc) (32 bytes),
wouldn't the size calculation immediately following this block underflow:

                size = info.gsp_rpc_len - sizeof(*rpc);
                expected -= size;
                info.gsp_rpc_buf += size;

Could this result in info.gsp_rpc_buf pointing to massively out-of-bounds
addresses and cause arbitrary memory corruption?

[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds
write inside r535_gsp_msgq_recv_one_elem() when handling wrapped
continuation messages?

For continuation messages, the first part of the copy skips the 32-byte
header, but the destination offset for the second part appears to be
incorrect:

        if (info->continuation)
                memcpy(buf, mqe->data + sizeof(struct nvfw_gsp_rpc),
                       len - sizeof(struct nvfw_gsp_rpc));
        else
                memcpy(buf, mqe->data, len);

        expected -= len;

        if (expected) {
                mqe = (void *)((u8 *)gsp->shm.msgq.ptr + ...);
                memcpy(buf + len, mqe, expected);

Since len isn't reduced by sizeof(struct nvfw_gsp_rpc) for the offset,
won't buf + len leave a 32-byte uninitialized hole and push the remainder
copy 32 bytes past the end of the allocated buffer?

> @@ -499,7 +499,7 @@ r535_gsp_msg_recv(struct nvkm_gsp *gsp, int fn, u32 gsp_rpc_len)
>  
>  retry:
>  	rpc = r535_gsp_msgq_peek(gsp, sizeof(*rpc), &retries);
> -	if (IS_ERR_OR_NULL(rpc))
> +	if (IS_ERR(rpc))
>  		return rpc;
>  
>  	rpc = r535_gsp_msgq_recv(gsp, gsp_rpc_len, &retries);

[Severity: High]
This is a pre-existing issue, but could r535_gsp_msgq_recv() return NULL
on oversized RPC messages?

Looking at r535_gsp_msgq_recv(), it returns NULL instead of an error
pointer when the WARN_ON triggers:

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

Many callers expect either a valid pointer or an ERR_PTR, and only check
for errors using IS_ERR(). For example, in r535_gsp_get_static_info():

        rpc = nvkm_gsp_rpc_rd(gsp, NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO, ...);
        if (IS_ERR(rpc))
                return PTR_ERR(rpc);

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

Since IS_ERR(NULL) is false, won't this result in a NULL pointer
dereference? Should r535_gsp_msgq_recv() return ERR_PTR(-E2BIG) instead?

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