Re: [PATCH v4 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] NULL pointer dereference and error masking when GSP firmware sends a malformed RPC message.
--

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

nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd()
    
nvkm_gsp_rpc_rd() calls nvkm_gsp_rpc_get() which never returns NULL,
only valid pointers or error pointers. Clean up by using IS_ERR()
instead, matching the actual return value contract.
    
> 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.

[Severity: High]
This isn't a bug introduced by this patch, but the commit message asserts
that nvkm_gsp_rpc_push() will return an error pointer instead of NULL under
the RECV policy. Is this assertion accurate?

If the GSP firmware sends a malformed message with an invalid size, the
receive path can return NULL even for RECV policies:

r535_gsp_msgq_recv() {
    ...
    if (WARN_ON(rpc->length > max_rpc_size))
        return NULL;
    ...
}

This NULL propagates up through r535_gsp_msg_recv() and
r535_gsp_rpc_handle_reply() to nvkm_gsp_rpc_push().

> 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);

Since this function returns the result of nvkm_gsp_rpc_push() directly, it
can return NULL.

Since callers of nvkm_gsp_rpc_rd() typically only check IS_ERR(rpc), wouldn't
this result in a NULL pointer dereference?

For example:

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.handle = rpc->hInternalClient;
}

Furthermore, in r535_gsp_rpc_rm_ctrl_push(), a NULL return triggers an
IS_ERR_OR_NULL check but incorrectly translates to a 0 (success) return via
PTR_ERR(rpc), masking the error completely.

Should r535_gsp_msgq_recv() be updated to return an ERR_PTR(-EIO) instead of
NULL when a malformed RPC size is encountered?

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