Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] `r535_gsp_msgq_recv()` incorrectly returns `NULL` instead of an `ERR_PTR` when receiving a message exceeding `max_rpc_size`, leading to NULL pointer dereferences in upstream callers. - [Medium] In `r535_gsp_rpc_push()`, when a large RPC payload encounters an error during transmission or waiting for a reply, the original message buffer is leaked. -- commit c28b7cbd3dab1fd32a31ef4df86d6f6aaf9ad40b Author: Hongling Zeng <[email protected]> nouveau/gsp/rm/bar: Cleanup IS_ERR_OR_NULL usage This commit updates r535_bar_bar2_update_pde() to use IS_ERR() instead of IS_ERR_OR_NULL() when checking the return value of nvkm_gsp_rpc_get(). This aligns with the function's contract of never returning 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 is a pre-existing issue, but there is a case where the GSP RPC infrastructure incorrectly returns a NULL pointer. If the message queue receives an unexpectedly large message: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv() { mqe = r535_gsp_msgq_get_entry(gsp); rpc = (struct nvfw_gsp_rpc *)mqe->data; if (WARN_ON(rpc->length > max_rpc_size)) return NULL; ... } This NULL return value propagates up through r535_gsp_msg_recv() and r535_gsp_rpc_handle_reply(), causing nvkm_gsp_rpc_rd() to return NULL. Does this lead to a NULL pointer dereference in callers like r535_gsp_get_static_info() since they only validate the result with IS_ERR()? 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; ... } [Severity: Medium] This is also a pre-existing issue, but does the slow path in r535_gsp_rpc_push() leak the original message buffer when an error occurs? drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() { ... if (payload_size > max_payload_size) { ... /* Send initial RPC. */ next = r535_gsp_rpc_get(gsp, fn, max_payload_size); if (IS_ERR(next)) { repv = next; goto done; } ... /* Wait for reply. */ repv = r535_gsp_rpc_handle_reply(gsp, fn, policy, payload_size + sizeof(*rpc)); if (!IS_ERR(repv)) kvfree(msg); } else { ... done: mutex_unlock(&gsp->cmdq.mutex); return repv; } In the fast path, r535_gsp_cmdq_push() takes ownership and unconditionally frees the buffer. However, in the chunked sending path above, if there is an error getting a chunk (IS_ERR(next)) or receiving a reply (IS_ERR(repv)), execution jumps to the done label without calling kvfree(msg). Could this permanently leak the large payload buffer? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
