[PATCH 0/1] accel/amdxdna: bound the firmware-supplied mailbox register offsets

[PATCH 0/1] accel/amdxdna: bound the firmware-supplied mailbox register offsets

[Eva Crystal]

This is a change at the firmware-to-driver trust boundary, and I would
rather be plain about that before anything else. The values in question
come from NPU firmware, not from userspace. There is no proof of
concept, I have reproduced nothing, and it is not established that the
part can report a mailbox register outside the mailbox aperture at all
-- I could not locate the producing code in the firmware image at
instruction level, so I make no claim about the emittable range, in
either direction. What I can show is static: the driver takes four
register offsets from firmware, computes and stores the exact bound
they should be checked against, and never checks them.

The shape of it. Firmware reports where a mailbox channel's head and
tail registers live: in the management mailbox block it writes into the
SRAM BAR, read by aie2_get_mgmt_chann_info(), and in the CREATE_CONTEXT
response, read by aie2_create_context() for every hardware context.
AIE2_MBOX_OFF() converts each to a raw byte offset into the mailbox
mapping. aie2_hw_start() and aie2_create_context() each derive the
mailbox interrupt register by adding 4 to one of them. On AIE4,
aie4_mailbox_start() takes four such offsets straight out of the
mailbox_info block. mailbox_reg_read() and mailbox_reg_write() then add
the offset to xdna_mailbox_res::mbox_base and hand the result to
readl()/writel(). Nothing in between compares it to anything.
AIE2_MBOX_OFF() is an unsigned 32-bit subtraction, so a reported address
below the aperture base does not fail closed but wraps to a very large
offset, and the + 4 can wrap independently of the value it derives from.

Why this is a patch rather than generic hardening: the driver already
treats firmware as an input to validate, in at least six places. It
rejects a bad management-mailbox magic (aie2_pci.c:93), checks the
reported protocol version (aie_check_protocol(), aie.c:68), range- and
alignment-checks the ring tail firmware writes (amdxdna_mailbox.c:294),
rejects an out-of-range device revision (aie2_message.c:1262), bounds a
reported error count against the buffer that has to hold it
(aie2_error.c:312), and -- the closest parallel -- bounds a
firmware-returned fw_ctx_id against priv->hwctx_limit before using it as
an array index, in aie2_fill_hwctx_map() at aie2_pci.c:917. That last
one is this patch in miniature: same source, same driver, a stored limit
consulted before use. The mailbox register offsets are the one
firmware-supplied value left unbounded, and the limit they should be
bounded against, xdna_mailbox_res::mbox_size, is assigned on the line
after the mbox_base they are added to, in the same struct, and then read
nowhere in the driver.

For precedent on the boundary itself -- not as a claim about this driver
-- accel/ivpu, the other NPU driver in accel/, took four fixes in 2026
whose entire content is a firmware-supplied value used without
validation. Each was assigned a CVE and each was backported across
several stable branches:

  commit d9faef564438 ("accel/ivpu: Fix signed integer truncation in
  IPC receive") -- CVE-2026-53202
  commit dd1311bcf0e6 ("accel/ivpu: Add bounds checks for firmware log
  indices") -- CVE-2026-53205
  commit 1d0b597facdd ("accel/ivpu: Add bounds check for firmware
  runtime memory") -- CVE-2026-53206
  commit ddb44baed257 ("accel/ivpu: Reject firmware log with size
  smaller than header") -- CVE-2026-72089

I cite them only for the principle that in this subsystem "firmware
reported it" has been treated as a reason to check a value rather than a
reason to trust it. They say nothing about whether this driver has a
real problem.

On impact, stated conservatively. This is not a controlled write
primitive and I am not presenting it as one. The offset is chosen by
firmware, not by an attacker: I have not shown any path by which
userspace influences what firmware puts in cq_info.head_addr, and I did
not look for one. The data written is not attacker-chosen either -- the
writes are a ring pointer the driver computed, or the constant 0 for the
interrupt acknowledge. The landing address is mbox_base + offset, where
mbox_base is a vmalloc-space address an attacker neither controls nor
observes. The realistic outcome of an out-of-range offset is an MMIO
access outside the ioremap: either a fault at a kernel virtual address,
from the rx workqueue or from the ioctl path, or a write into whatever
else is mapped there. On parts where the mailbox BAR is also the public
register BAR, an overshoot that stays inside the BAR reaches public
registers instead of mailbox head and tail. That is a landing-zone
detail, not an impact upgrade.

The patch itself is one check, in xdna_mailbox_start_channel(). I
considered putting the bound in mailbox_reg_read()/mailbox_reg_write()
instead, since that is where an offset meets readl()/writel(), but the
six call sites argue against it: two return void and two return a u32 in
which every value is a legal register value, so a guard there could only
silently skip a write or fabricate a read, and it would sit in the
per-message IO path. All six take their offset from mb_chann->res[] or
mb_chann->iohub_int_addr, and xdna_mailbox_start_channel() is the only
writer of either, so it is a genuine choke point: the management
channel, every hardware context and the AIE4 path all pass through it,
and the derived interrupt register arrives there as a parameter, which a
check at the producing sites would not cover. It already validates the
ring sizes, it already returns int, and all three callers handle a
failure -- aie2_hw_start() and aie4_mailbox_start() unwind and fail the
probe or resume, and aie2_create_context() frees the channel and
destroys the firmware context, so AMDXDNA_CREATE_HWCTX returns an error
to userspace rather than leaving a channel that accesses outside its
mapping.

No Fixes: tag, deliberately. The defect dates to the driver's initial
merge in v6.14-rc1, but this patch does not apply to any of the three
commits that introduced it, and I checked rather than assumed: at that
point the accessors used a u64 address with (void *) casts, and the
function this patch changes did not exist -- it was
xdna_mailbox_create_channel(), returning a pointer and NULL on error
rather than an int. A Fixes: tag would point at trees the patch cannot
be applied to without being rewritten. Say the word if you would rather
have the provenance recorded and deal with the backport separately.

Based on commit d681d7ef617e ("Merge misc regression fixes that seem to
have fallen through the cracks"). Builds clean on x86_64 with
CONFIG_DRM_ACCEL_AMDXDNA=m, gcc 15.3, W=1, no new warnings;
checkpatch --strict reports nothing. Not tested on hardware: I have no
way to make firmware report an out-of-range offset, which is the same
gap described at the top.

Eva Crystal (1):
  accel/amdxdna: bound the firmware-supplied mailbox register offsets

 drivers/accel/amdxdna/amdxdna_mailbox.c | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

-- 
2.53.0

[PATCH 1/1] accel/amdxdna: bound the firmware-supplied mailbox register offsets

[Eva Crystal]

Firmware chooses where a mailbox channel's head and tail registers live
and reports them to the driver as device addresses: in the management
mailbox block it writes into the SRAM BAR, read by
aie2_get_mgmt_chann_info(), and in the CREATE_CONTEXT response, read by
aie2_create_context() for every hardware context. AIE2_MBOX_OFF() turns
each one into a raw byte offset into the mailbox mapping, and both
aie2_hw_start() and aie2_create_context() derive the mailbox interrupt
register from one of them by adding 4. On AIE4, aie4_mailbox_start()
takes the same four offsets straight out of the mailbox_info block.

None of them is checked. mailbox_reg_read() and mailbox_reg_write() add
the offset to xdna_mailbox_res::mbox_base and hand the result to
readl()/writel(), so an offset past the end of that mapping is an MMIO
access outside it, at a kernel virtual address the driver has no claim
to. AIE2_MBOX_OFF() is an unsigned 32-bit subtraction: a reported
address below the aperture base does not yield an obviously invalid
small offset but wraps to a very large one, and the + 4 for the
interrupt register can wrap independently of the value it derives from.

The bound is already there. xdna_mailbox_res::mbox_size is the exact
length of the mapping pcim_iomap() produced - the device's mailbox
window size, or the BAR length when the device does not override it -
and it sits in the same struct as mbox_base. The driver stores it and
never reads it.

Check the four register offsets and the interrupt register against
mbox_size in xdna_mailbox_start_channel(). Every mailbox register
access reads mb_chann->res[] or mb_chann->iohub_int_addr, and that
function is the only writer of either, so it is the one point every
firmware-supplied offset passes through, for the management channel,
for a hardware context and for AIE4 alike. It is also where the derived
interrupt register arrives, as a parameter, which a check at the
producing sites would not cover. A zero interrupt register keeps its
existing meaning of "this platform has no such register".

Failing there rejects the channel before any offset reaches readl() or
writel(). aie2_hw_start() and aie4_mailbox_start() unwind and fail the
probe or resume; aie2_create_context() frees the channel and destroys
the firmware context, so AMDXDNA_CREATE_HWCTX returns an error to
userspace instead of leaving a channel that accesses outside its
mapping.

This is the firmware-to-driver trust boundary. Whether the part can
report a register outside the mailbox aperture is not established and
there is no reproducer. The offsets are simply the one firmware-supplied
value this driver leaves unbounded, against a limit it already computes
and stores.

Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
 drivers/accel/amdxdna/amdxdna_mailbox.c | 32 +++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index cc8865f4e79c..0c4cb8beb26e 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;
+}
+
+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);
+}
+
 static inline void mailbox_irq_acknowledge(struct mailbox_channel *mb_chann)
 {
 	if (mb_chann->iohub_int_addr)
@@ -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))) {
+		dev_err(mb_chann->mb->dev,
+			"Mailbox register offset outside the %zu byte mailbox mapping\n",
+			mb_chann->mb->res.mbox_size);
+		return -EINVAL;
+	}
+
 	mb_chann->msix_irq = mb_irq;
 	mb_chann->iohub_int_addr = iohub_int_addr;
 	memcpy(&mb_chann->res[CHAN_RES_X2I], x2i, sizeof(*x2i));
-- 
2.53.0

Re: [PATCH 1/1] accel/amdxdna: bound the firmware-supplied mailbox register offsets

[sashiko-bot]

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 <0xiviel@gmail.com>

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/20260913200212.133126-2-0xiviel@gmail.com?part=1