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 <[email protected]> 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/[email protected]?part=5
