Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] Memory leak of the RPC message buffer in r535_gsp_rpc_push() when an error occurs while sending large payloads. - [High] Malformed GSP firmware messages cause a NULL pointer dereference via nvkm_gsp_rm_ctrl_rd(). -- commit f3a9eeee67cd9b42fd2bbb4cf7eff6681fafba12 Author: Hongling Zeng <[email protected]> nouveau/gsp/rm/bar: Cleanup IS_ERR_OR_NULL usage This commit cleans up the usage of IS_ERR_OR_NULL in r535_bar_bar2_update_pde() by using IS_ERR() instead, matching the actual return value contract of nvkm_gsp_rpc_get() which never returns 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 isn't a bug introduced by this patch, but while reviewing this cleanup, I noticed another IS_ERR_OR_NULL usage in r535_gsp_rpc_rm_ctrl_push() that causes a NULL pointer dereference. When a malformed GSP firmware message is received: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv() { ... if (WARN_ON(rpc->length > max_rpc_size)) return NULL; ... } This NULL propagates to r535_gsp_rpc_rm_ctrl_push(): drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/ctrl.c:r535_gsp_rpc_rm_ctrl_push() { ... rpc = nvkm_gsp_rpc_push(gsp, rpc, NVKM_GSP_RPC_REPLY_RECV, repc); if (IS_ERR_OR_NULL(rpc)) { *params = NULL; return PTR_ERR(rpc); } ... } Does PTR_ERR(rpc) evaluate to 0 here when rpc is NULL? If so, this erroneously signals success to callers like nvkm_gsp_rm_ctrl_rd(), which will then blindly dereference the NULL pointer. Could this be handled differently so an error code is returned instead? [Severity: High] This isn't a bug introduced by this patch, but looking at r535_gsp_rpc_push(), could there be a memory leak of the RPC message buffer when an error occurs while sending large payloads? drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() { ... if (payload_size > max_payload_size) { ... while (remain_payload_size) { u32 size = min(remain_payload_size, max_payload_size); next = r535_gsp_rpc_get(gsp, NV_VGPU_MSG_FUNCTION_CONTINUATION_RECORD, size); if (IS_ERR(next)) { repv = next; goto done; } ... if (!IS_ERR(repv)) kvfree(msg); } else { ... done: mutex_unlock(&gsp->cmdq.mutex); return repv; } When IS_ERR(next) is true during chunking, the execution jumps to the done label, bypassing the kvfree(msg) cleanup logic. The caller function then sets its pointer to NULL on error, losing the reference to the original msg buffer. Does this result in a resource leak on this error path? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
