Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [High] Returning NULL on an invalid RPC message length deadlocks the message queue and causes downstream wrappers to treat errors as success, leading to NULL pointer dereferences. - [High] Lack of bounds checking on continuation chunks allows a malformed or malicious GSP message to cause integer underflows and heap buffer overflows. - [High] The original split RPC message buffer is leaked on error paths when the payload exceeds max_payload_size. -- commit b2d404f83fcf801450f3eade502dc33e20d3fbe6 Author: Hongling Zeng <[email protected]> Subject: nouveau/gsp/rpc: Document RPC function return value contracts This commit adds kernel-doc comments to document the return value semantics of RPC functions in the Nouveau GSP RM r535/rpc.c file. > diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > index 3ca3de8f43408..2ca6de4094e63 100644 > --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c > +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c [ ... ] > @@ -283,6 +300,15 @@ r535_gsp_msgq_recv_one_elem(struct nvkm_gsp *gsp, > return buf; > } > > +/** > + * r535_gsp_msgq_recv - Receive RPC message(s) from GSP queue > + * @gsp: GSP device > + * @gsp_rpc_len: Expected RPC length > + * @retries: Retry counter > + * > + * Return: Pointer to received buffer on success, ERR_PTR() on error, > + * or NULL if RPC length is invalid. > + */ > static void * > r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, int *retries) > { [Severity: High] This is a pre-existing issue, but does returning NULL here when the RPC length is invalid cause the message queue to deadlock and trigger NULL pointer dereferences downstream? Looking at r535_gsp_msgq_recv(), it returns NULL without advancing the read pointer: if (WARN_ON(rpc->length > max_rpc_size)) return NULL; Several downstream functions seem to mishandle this NULL return. For example, in r535_gsp_rpc_rm_ctrl_push(): if (IS_ERR_OR_NULL(rpc)) { *params = NULL; return PTR_ERR(rpc); } This converts IS_ERR_OR_NULL(NULL) to PTR_ERR(NULL) == 0, signaling success to the caller while setting *params to NULL. Similarly, nvkm_gsp_rm_alloc_wr() checks IS_ERR(repv) and returns success since IS_ERR is false for NULL. Callers like r535_gsp_get_static_info() then check the reply with IS_ERR() instead of IS_ERR_OR_NULL(), leading directly to a NULL pointer dereference. [Severity: High] This is also a pre-existing issue in r535_gsp_msgq_recv(), but can a malformed GSP message cause integer underflows and heap overflows during continuation chunk processing? In the continuation loop: info.gsp_rpc_len = rpc->length; info.continuation = true; rpc = r535_gsp_msgq_recv_one_elem(gsp, &info); ... size = info.gsp_rpc_len - sizeof(*rpc); expected -= size; If rpc->length is less than sizeof(*rpc), the subtraction for size will underflow to a massive value, which then appears to cause an out-of-bounds memcpy in r535_gsp_msgq_recv_one_elem(). Is there a missing bounds check against the remaining expected capacity and the minimum header size? [ ... ] > @@ -628,6 +704,17 @@ r535_gsp_rpc_get(struct nvkm_gsp *gsp, u32 fn, u32 > payload_size) > return rpc->data; > } > > +/** > + * r535_gsp_rpc_push - Push RPC message to GSP and wait for reply > + * @gsp: GSP device > + * @payload: RPC payload to send > + * @policy: Reply policy (NOWAIT, NOSEQ, RECV, or POLL) > + * @gsp_rpc_len: Expected RPC length in the reply > + * > + * Return: NULL when policy is NOWAIT/NOSEQ (no reply expected) or > + * POLL (successful message processing), pointer to reply data > + * on success, or ERR_PTR() on error. > + */ > static void * > r535_gsp_rpc_push(struct nvkm_gsp *gsp, void *payload, > enum nvkm_gsp_rpc_reply_policy policy, u32 gsp_rpc_len) [Severity: High] This is a pre-existing issue, but does this function leak the original split RPC message buffer on error paths? In r535_gsp_rpc_push(), when the payload exceeds max_payload_size, it is chunked and sent iteratively. If an intermediate chunk fails to send, the code jumps to the done label: repv = r535_gsp_rpc_send(gsp, next, NVKM_GSP_RPC_REPLY_NOWAIT, 0); if (IS_ERR(repv)) goto done; ... if (!IS_ERR(repv)) kvfree(msg); done: mutex_unlock(&gsp->cmdq.mutex); return repv; Because kvfree(msg) is only executed if the operations are successful (!IS_ERR(repv)), does the original msg buffer remain permanently leaked when an error occurs? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
