Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues: - [Critical] Integer underflow in `r535_gsp_msgq_recv_one_elem` due to missing lower bounds check on `rpc->length` causes massive out-of-bounds memory copy. - [Critical] A Time-of-Check to Time-of-Use (TOCTOU) double-fetch vulnerability in `r535_gsp_msgq_recv` allows a malicious GSP firmware to cause a kernel heap buffer overflow. - [High] NULL pointer dereference when `r535_gsp_msgq_recv()` encounters an invalid RPC message size. - [High] Out-of-bounds memory reads due to unvalidated array counts from firmware in `r535_gsp_intr_get_table` and `r535_gsp_get_static_info_fb`. -- commit 88b24214ef8688e688f55f1aef5cd0f776cdce80 Author: Hongling Zeng <[email protected]> Subject: nouveau/gsp: Cleanup IS_ERR_OR_NULL in nvkm_gsp_rpc_rd() This commit cleans up the error checking in nvkm_gsp_rpc_rd(). Since nvkm_gsp_rpc_get() returns only valid pointers or error pointers, the code is updated to use IS_ERR() instead of IS_ERR_OR_NULL() to match the actual API 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: High] This isn't a bug introduced by this patch, but if r535_gsp_msgq_recv() returns NULL when rpc->length exceeds max_rpc_size, doesn't this bubble up through nvkm_gsp_rpc_push() and nvkm_gsp_rpc_rd()? If callers like r535_gsp_get_static_info() only check IS_ERR(), won't they dereference NULL? 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; ... } Should r535_gsp_msgq_recv() return an error pointer like ERR_PTR(-EIO) instead of NULL to match this API contract? [Severity: High] This is a pre-existing issue, but does r535_gsp_intr_get_table() properly bound its iteration over ctrl->tableLen? If a faulty firmware provides a count larger than NV2080_CTRL_INTERNAL_INTR_MAX_TABLE_SIZE, won't this loop read out-of-bounds into adjacent memory? Similarly, does r535_gsp_get_static_info_fb() need to verify info->numFBRegions against the fbRegion array bounds before iterating? [Severity: Critical] This isn't a bug introduced by this patch, but is there a missing lower bounds check for rpc->length in r535_gsp_msgq_recv()? If rpc->length is 0, len evaluates to 0 for continuation records in r535_gsp_msgq_recv_one_elem(). Doesn't len - sizeof(struct nvfw_gsp_rpc) then underflow, causing a massive out-of-bounds memory copy? drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r535/rpc.c:r535_gsp_msgq_recv_one_elem() { ... if (info->continuation) memcpy(buf, mqe->data + sizeof(struct nvfw_gsp_rpc), len - sizeof(struct nvfw_gsp_rpc)); ... } [Severity: Critical] This is a pre-existing issue, but does r535_gsp_msgq_recv() safely handle rpc->length from shared memory? It appears the firmware-provided rpc->length is read directly from mqe->data three times. If a malicious firmware increases this value after the bounds check and allocation, but before info.gsp_rpc_len = rpc->length, couldn't r535_gsp_msgq_recv_one_elem() cause a heap overflow when copying up to info.gsp_rpc_len bytes? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
