Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
Pre-existing issues: - [Critical] Dangling stack pointer dereference and arbitrary memory overwrite when a synchronous mailbox message times out - [High] Incomplete bounds checking for firmware-supplied mailbox ring buffer offsets - [High] Missing alignment check for firmware-supplied MMIO register offsets - [High] Dangling pointer in the error path of aie2_create_context() leads to Use-After-Free and Double Free - [High] Memory and XArray ID leak when firmware does not respond to a mailbox message, leading to a rapid Denial of Service -- commit 6dc11363e9e0c2c2b0493330df8c49c994026bf8 Author: Eva Crystal <[email protected]> accel/amdxdna: bound the firmware-supplied mailbox register offsets Firmware chooses where a mailbox channel's head and tail registers live and reports them to the driver as device addresses. This patch adds checks to ensure the four register offsets and the interrupt register are within the mailbox mapping size before use. > diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c > b/drivers/accel/amdxdna/amdxdna_mailbox.c > index cc8865f4e79c9..0c4cb8beb26ea 100644 > --- a/drivers/accel/amdxdna/amdxdna_mailbox.c > +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c > @@ -112,6 +112,28 @@ static u32 mailbox_reg_read(struct mailbox_channel > *mb_chann, u32 mbox_reg) > return readl(ringbuf_addr); > } > > +/* > + * Firmware describes where a channel's head and tail registers live, as raw > + * offsets into the mailbox mapping: in the management mailbox block it > writes > + * into SRAM for the management channel, and in the CREATE_CONTEXT response > for > + * a hardware context. Both helpers above add such an offset straight to > + * mbox_base, so bound it against the size of that mapping first. > + */ > +static bool mailbox_reg_in_range(struct mailbox_channel *mb_chann, u32 > mbox_reg) > +{ > + struct xdna_mailbox_res *mb_res = &mb_chann->mb->res; > + > + /* Every access through the two helpers above is 32 bits wide. */ > + return (u64)mbox_reg + sizeof(u32) <= mb_res->mbox_size; > +} [Severity: High] This is a pre-existing issue, but since this bounds check validates the offset magnitude, should it also enforce the natural alignment IS_ALIGNED(mbox_reg, 4) required for 32-bit MMIO accesses? If the firmware provides an unaligned register offset (e.g., 0x1), could this lead to hardware alignment faults, Machine Check Exceptions, or PCIe AER faults when readl() or writel() are called? > + > +static bool mailbox_chann_res_in_range(struct mailbox_channel *mb_chann, > + const struct xdna_mailbox_chann_res *res) > +{ > + return mailbox_reg_in_range(mb_chann, res->mb_head_ptr_reg) && > + mailbox_reg_in_range(mb_chann, res->mb_tail_ptr_reg); > +} [ ... ] > @@ -518,6 +540,16 @@ xdna_mailbox_start_channel(struct mailbox_channel > *mb_chann, > return -EINVAL; > } > > + /* A zero iohub_int_addr means the platform has no such register. */ > + if (!mailbox_chann_res_in_range(mb_chann, x2i) || > + !mailbox_chann_res_in_range(mb_chann, i2x) || > + (iohub_int_addr && !mailbox_reg_in_range(mb_chann, > iohub_int_addr))) { [Severity: High] This is a pre-existing issue, but the patch attempts to establish a firmware-to-driver trust boundary for mailbox offsets and completely omits the ring buffer limits here. Does this code miss checking x2i->rb_start_addr and i2x->rb_start_addr against the mapped ringbuf_size? If firmware provides a malicious rb_start_addr, it will be used in mailbox_send_msg() directly to compute an MMIO address: drivers/accel/amdxdna/amdxdna_mailbox.c:mailbox_send_msg() { ... write_addr = mb_chann->mb->res.ringbuf_base + start_addr + tail; writel(TOMBSTONE, write_addr); ... } Furthermore, if the firmware sets rb_size to a tiny power of two (e.g., 2), mailbox_get_ringbuf_size() - sizeof(u32) severely integer-underflows, breaking the ring buffer wrap-around checks. Initial x2i_tail and i2x_head are also read directly from device registers without validation, leading to immediate out-of-bounds MMIO accesses. > + dev_err(mb_chann->mb->dev, > + "Mailbox register offset outside the %zu byte mailbox > mapping\n", > + mb_chann->mb->res.mbox_size); > + return -EINVAL; [Severity: High] This is a pre-existing issue, but adding this return -EINVAL actively exposes a dangling pointer in aie2_create_context(). If xdna_mailbox_start_channel() returns -EINVAL here, aie2_create_context() jumps to its error path and frees the channel without clearing the pointer: drivers/accel/amdxdna/aie2_message.c:aie2_create_context() { ... free_channel: xdna_mailbox_free_channel(hwctx->priv->mbox_chann); del_ctx_req: ... } If context creation fails, aie2_xrs_unload() subsequently calls aie2_destroy_context(), which evaluates the dangling pointer: drivers/accel/amdxdna/aie2_message.c:aie2_destroy_context() { ... if (!hwctx->priv->mbox_chann) return 0; xdna_mailbox_stop_channel(hwctx->priv->mbox_chann); ret = aie2_destroy_context_req(ndev, hwctx->fw_ctx_id); xdna_mailbox_free_channel(hwctx->priv->mbox_chann); ... } Can this trigger a Use-After-Free and Double Free? > + } > + > mb_chann->msix_irq = mb_irq; > mb_chann->iohub_int_addr = iohub_int_addr; > memcpy(&mb_chann->res[CHAN_RES_X2I], x2i, sizeof(*x2i)); [Severity: Critical] This is a pre-existing issue, but I also noticed a potential vulnerability with synchronous mailbox messages. Does xdna_send_msg_wait() leave a time-bomb callback pointing to the stack if it times out? drivers/accel/amdxdna/amdxdna_mailbox_helper.c:xdna_send_msg_wait() { ... ret = wait_for_completion_timeout(&hdl->comp, msecs_to_jiffies(RX_TIMEOUT)); if (!ret) { XDNA_ERR(xdna, "Wait for completion timeout"); return -ETIME; } ... } If this times out, the function returns and destroys its stack frame, but the pending message remains in the mailbox XArray. When the device firmware eventually writes a delayed response, the RX worker calls xdna_msg_cb() with the dangling stack pointer: drivers/accel/amdxdna/amdxdna_mailbox_helper.c:xdna_msg_cb() { ... memcpy_fromio(cb_arg->data, data, cb_arg->size); ... } Could this overwrite arbitrary kernel memory using corrupted size and data fields from the destroyed stack frame? [Severity: High] This is a pre-existing issue, but related to the timeout behavior above, does xdna_mailbox_send_msg() leak memory and XArray IDs when a timeout occurs? drivers/accel/amdxdna/amdxdna_mailbox.c:xdna_mailbox_send_msg() { ... ret = mailbox_send_msg(mb_chann, mb_msg); if (ret) { MB_DBG(mb_chann, "Error in mailbox send msg, ret %d", ret); goto release_id; } return 0; ... } When xdna_send_msg_wait() times out, it fails to release the XArray ID or free the dynamically allocated mb_msg. Since XArray IDs are limited to 256 entries for the mailbox channel, could an aggressive user repeatedly trigger timeouts to consume all 256 slots? Once exhausted, mailbox_acquire_msgid() will indefinitely fail with -ENOSPC, permanently causing a Denial of Service on the mailbox channel until the driver is reloaded. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
