Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] NULL pointer dereference when GSP firmware sends a malformed message with invalid length. - [Medium] Memory leak of large RPC request buffers on error paths. -- commit 71d7ed95d2b3f836cf2bbe162c7b67d89c46964e Author: Hongling Zeng <[email protected]> nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd() This patch changes nvkm_gsp_rpc_rd() to check IS_ERR() instead of IS_ERR_OR_NULL() for the return value of nvkm_gsp_rpc_get(). It also notes that nvkm_gsp_rpc_push() should return error pointers, not NULL, when using the RECV policy. > 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. > > Signed-off-by: Hongling Zeng <[email protected]> [Severity: High] This is a pre-existing issue, but is the assumption in the commit message accurate? Looking at r535_gsp_msgq_recv() in rpc.c, if a malformed message is received, it explicitly returns NULL: 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 through the reply handlers to nvkm_gsp_rpc_push(), which returns it to the caller. Because callers like r535_gsp_get_static_info() only check IS_ERR(), this leads to a NULL pointer dereference: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/gsp.c:r535_gsp_get_static_info() { if (IS_ERR(rpc)) return PTR_ERR(rpc); gsp->internal.client.object.client = &gsp->internal.client; ... } Other callers like r535_gsp_rpc_rm_ctrl_push() implicitly convert this NULL return to 0 (success) via PTR_ERR(rpc), masking the failure. Should the underlying r535_gsp_msgq_recv() be modified to return an ERR_PTR to match the expected contract? > 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); ^^^^ [Severity: Medium] This is a pre-existing issue, but does r535_gsp_rpc_push() leak the msg buffer on error paths? Looking at r535_gsp_rpc_push() in rpc.c, when payload_size > max_payload_size, the large msg buffer is freed only on success: drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_rpc_push() { ... done: if (!IS_ERR(repv)) kvfree(msg); return repv; } If any intermediate chunk allocation fails, or if the final reply results in an error pointer, could this leak the large RPC request buffers? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
