Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues: - [Critical] Missing validation of `rpc->length` from untrusted GSP firmware leads to integer underflows and massive out-of-bounds memory accesses. - [High] Incorrect destination offset when a continuation message wraps around the ring buffer leads to payload corruption and an out-of-bounds write. - [High] `r535_gsp_msgq_recv()` returns `NULL` on oversized RPC messages, leading to NULL pointer dereferences in callers. -- commit c55dadd139cb2f5047a2ad1948afcbd89e5133ab Author: Hongling Zeng <[email protected]> nouveau/gsp/rpc: Cleanup incorrect IS_ERR_OR_NULL in rpc.c Clean up incorrect IS_ERR_OR_NULL() usage for functions that never return NULL: - r535_gsp_msgq_peek() never returns NULL - r535_gsp_msgq_recv_one_elem() never returns NULL These functions should be checked with IS_ERR() instead. > 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 35272c1538699..25d3adbc13550 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 [ ... ] > @@ -359,7 +359,7 @@ r535_gsp_msgq_recv(struct nvkm_gsp *gsp, u32 gsp_rpc_len, > int *retries) > info.continuation = true; > > rpc = r535_gsp_msgq_recv_one_elem(gsp, &info); > - if (IS_ERR_OR_NULL(rpc)) { > + if (IS_ERR(rpc)) { > kvfree(buf); > return rpc; > } [Severity: Critical] This is a pre-existing issue, but does r535_gsp_msgq_recv() validate the size of rpc->length coming from the untrusted GSP firmware? If a compromised GSP sends an rpc->length less than sizeof(*rpc) (32 bytes), wouldn't the size calculation immediately following this block underflow: size = info.gsp_rpc_len - sizeof(*rpc); expected -= size; info.gsp_rpc_buf += size; Could this result in info.gsp_rpc_buf pointing to massively out-of-bounds addresses and cause arbitrary memory corruption? [Severity: High] This isn't a bug introduced by this patch, but is there an out-of-bounds write inside r535_gsp_msgq_recv_one_elem() when handling wrapped continuation messages? For continuation messages, the first part of the copy skips the 32-byte header, but the destination offset for the second part appears to be incorrect: if (info->continuation) memcpy(buf, mqe->data + sizeof(struct nvfw_gsp_rpc), len - sizeof(struct nvfw_gsp_rpc)); else memcpy(buf, mqe->data, len); expected -= len; if (expected) { mqe = (void *)((u8 *)gsp->shm.msgq.ptr + ...); memcpy(buf + len, mqe, expected); Since len isn't reduced by sizeof(struct nvfw_gsp_rpc) for the offset, won't buf + len leave a 32-byte uninitialized hole and push the remainder copy 32 bytes past the end of the allocated buffer? > @@ -499,7 +499,7 @@ r535_gsp_msg_recv(struct nvkm_gsp *gsp, int fn, u32 > gsp_rpc_len) > > retry: > rpc = r535_gsp_msgq_peek(gsp, sizeof(*rpc), &retries); > - if (IS_ERR_OR_NULL(rpc)) > + if (IS_ERR(rpc)) > return rpc; > > rpc = r535_gsp_msgq_recv(gsp, gsp_rpc_len, &retries); [Severity: High] This is a pre-existing issue, but could r535_gsp_msgq_recv() return NULL on oversized RPC messages? Looking at r535_gsp_msgq_recv(), it returns NULL instead of an error pointer when the WARN_ON triggers: if (WARN_ON(rpc->length > max_rpc_size)) return NULL; Many callers expect either a valid pointer or an ERR_PTR, and only check for errors using IS_ERR(). For example, in r535_gsp_get_static_info(): rpc = nvkm_gsp_rpc_rd(gsp, NV_VGPU_MSG_FUNCTION_GET_GSP_STATIC_INFO, ...); if (IS_ERR(rpc)) return PTR_ERR(rpc); gsp->internal.client.object.client = &gsp->internal.client; gsp->internal.client.object.parent = NULL; gsp->internal.client.object.handle = rpc->hInternalClient; Since IS_ERR(NULL) is false, won't this result in a NULL pointer dereference? Should r535_gsp_msgq_recv() return ERR_PTR(-E2BIG) instead? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
