Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] The patch fails to resolve the TOCTOU vulnerability because it issues multiple independent `READ_ONCE(cmd->header)` calls instead of storing a single local snapshot. Pre-existing issues: - [High] Undefined behavior caused by passing a `NULL` source pointer to `memcpy()` when parsing an empty command payload. - [High] Uninitialized kernel stack memory is sent to the hardware mailbox, causing an information leak. -- commit c8e0faf1355b7f70321f8abedf7d99ec0ac63c46 Author: Taimuraz Kaitmazov <[email protected]> accel/amdxdna: use READ_ONCE/WRITE_ONCE on the command header This commit attempts to fix a TOCTOU race condition in the command header parsing by using READ_ONCE and WRITE_ONCE macros. It aims to read the user-mapped command header once into a local variable to prevent the compiler from splitting loads. > diff --git a/drivers/accel/amdxdna/amdxdna_ctx.c > b/drivers/accel/amdxdna/amdxdna_ctx.c > index 888e857ec5582..02027522210d9 100644 > --- a/drivers/accel/amdxdna/amdxdna_ctx.c > +++ b/drivers/accel/amdxdna/amdxdna_ctx.c > @@ -123,10 +123,11 @@ void *amdxdna_cmd_get_payload(struct amdxdna_gem_obj > *abo, u32 *size) > if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) > num_masks = 0; > else > - num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, > cmd->header); > + num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, > + READ_ONCE(cmd->header)); > > if (size) { > - count = FIELD_GET(AMDXDNA_CMD_COUNT, cmd->header); > + count = FIELD_GET(AMDXDNA_CMD_COUNT, READ_ONCE(cmd->header)); [Severity: High] Does this actually fix the race condition? The commit message explicitly claims to fix this by reading the header "once into a local with READ_ONCE()". Because READ_ONCE() enforces a volatile memory access, the compiler is forced to issue multiple distinct memory loads for the opcode, EXTRA_CU_MASK, and COUNT fields. Could the fields still be fetched from different snapshots of the header, maintaining the TOCTOU vulnerability? > if (unlikely(count <= num_masks || > count * sizeof(u32) + > offsetof(struct amdxdna_cmd, data[0]) > > @@ -151,7 +152,7 @@ u32 amdxdna_cmd_get_cu_idx(struct amdxdna_gem_obj *abo) > if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) > return INVALID_CU_IDX; > > - num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, cmd->header); > + num_masks = 1 + FIELD_GET(AMDXDNA_CMD_EXTRA_CU_MASK, > READ_ONCE(cmd->header)); [Severity: High] Does this suffer from the same issue? amdxdna_cmd_get_op() already reads the header, and here it is read again with a separate READ_ONCE(), rather than using a single local variable. [ ... ] > @@ -169,12 +170,15 @@ int amdxdna_cmd_set_error(struct amdxdna_gem_obj *abo, > struct amdxdna_client *client = job->hwctx->client; > struct amdxdna_cmd *cmd = amdxdna_gem_vmap(abo); > struct amdxdna_cmd_chain *cc = NULL; > + u32 header; > > if (!cmd) > return -ENOMEM; > > - cmd->header &= ~AMDXDNA_CMD_STATE; > - cmd->header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); > + header = READ_ONCE(cmd->header); > + header &= ~AMDXDNA_CMD_STATE; > + header |= FIELD_PREP(AMDXDNA_CMD_STATE, error_state); > + WRITE_ONCE(cmd->header, header); > > if (amdxdna_cmd_get_op(abo) == ERT_CMD_CHAIN) { [Severity: High] Does this code read the header again after modifying it? amdxdna_cmd_get_op() internally does a READ_ONCE(cmd->header), which happens right after the write. Could it end up reading a newly injected opcode if userspace modifies the header concurrently? [Severity: High] This isn't a bug introduced by this patch, but could this trigger undefined behavior if a user passes an empty payload? If a userspace application configures a command buffer with count <= num_masks, amdxdna_cmd_get_payload() can validly return NULL: drivers/accel/amdxdna/amdxdna_ctx.c:amdxdna_cmd_get_payload() { ... if (unlikely(count <= num_masks || count * sizeof(u32) + offsetof(struct amdxdna_cmd, data[0]) > abo->mem.size)) { *size = 0; return NULL; } ... } In drivers/accel/amdxdna/aie2_message.c:aie2_init_exec_cu_req(), the returned pointer is passed directly to memcpy() without validation: cmd = amdxdna_cmd_get_payload(cmd_bo, &cmd_len); if (cmd_len > sizeof(cu_req->payload)) return -EINVAL; cu_req->cu_idx = amdxdna_cmd_get_cu_idx(cmd_bo); if (cu_req->cu_idx == INVALID_CU_IDX) return -EINVAL; memcpy(cu_req->payload, cmd, cmd_len); Passing a NULL pointer to memcpy() triggers undefined behavior according to the C standard, even if the length is 0. Will this cause a kernel panic if panic_on_warn is enabled when UBSAN catches this? [Severity: High] This is a pre-existing issue, but does this leak uninitialized stack memory to the hardware mailbox? In drivers/accel/amdxdna/aie2_message.c:aie2_execbuf(), an uninitialized union is allocated on the stack: union exec_req req; ... ret = aie2_init_exec_req(&req, cmd_abo, &msg.send_size, &msg.opcode); If aie2_init_exec_cu_req() processes a small payload (e.g. cmd_len = 0), the remainder of the payload array retains uninitialized stack contents: memcpy(cu_req->payload, cmd, cmd_len); *size = sizeof(*cu_req); Could the remaining 76 bytes of uninitialized kernel stack memory be transmitted when xdna_mailbox_send_msg() is called? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
