* [PATCH] accel/amdxdna: bound the firmware-supplied mailbox ring against the mapped region
@ 2026-10-02 6:37 Eva Crystal
2026-10-02 16:34 ` Lizhi Hou
0 siblings, 1 reply; 3+ messages in thread
From: Eva Crystal @ 2026-10-02 6:37 UTC (permalink / raw)
To: Min Ma, Lizhi Hou, Oded Gabbay
Cc: dri-devel, linux-kernel, sashiko-reviews, Eva Crystal, Sashiko AI review
The mailbox ring start address and size both come from firmware, and
rb_start_addr is used directly as an offset into the ioremapped SRAM BAR:
read_addr = mb_chann->mb->res.ringbuf_base + start_addr + head;
header.total_size = readl(read_addr);
Nothing compares it against the length of that mapping. The only gate
between the firmware words and that readl() is is_power_of_2(rb_size) in
xdna_mailbox_start_channel(); the bounds tests in mailbox_get_msg() are
all relative to the firmware-supplied rb_size, not to the mapping. The
driver does record the true length, in struct
xdna_mailbox_res::ringbuf_size from pci_resource_len(), but that field is
never read anywhere.
So firmware can describe a ring that starts past the end of the BAR, and
the receive path reads there. Measured on an npu4 device (0000:06:00.1,
1022:17f0 rev 0x10, fw 1.1.2.64) by moving i2x_buf one page beyond the
window: the channel starts, the first firmware response drives
mailbox_get_msg(), and the peek readl() in mailbox_rx_worker faults on a
not-present page at exactly ringbuf_base + ringbuf_size. pcim_iomap()
maps the whole BAR, so one byte past the window is already past the
mapping. This happens during probe, before any device node exists, so no
userspace is involved. The oops kills the mailbox worker mid work item
and probe then wedges in drain_workqueue(). Only the read direction was
measured; nothing is claimed here about the write path.
Reject a channel whose ring does not lie entirely inside the mapping.
The check sits in xdna_mailbox_start_channel(), which is where all three
producers of this geometry converge: the management information block in
aie2_get_mgmt_chann_info(), the CREATE_CONTEXT response in
aie2_create_context(), and the mailbox information block in
aie4_mailbox_start().
Reaching this needs firmware that reports geometry it should not.
npu*.sbin is signed and loaded from root-owned /lib/firmware, so this is
not something an unprivileged user can drive. The file already validates
other firmware input on this path, the MGMT_MBOX_MAGIC check,
aie2_check_protocol() and the tail pointer in mailbox_get_msg(), so the
standard applied here is the one it already sets for itself.
Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/all/20260913214451.639251F000FF@smtp.kernel.org/
Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
This was first raised publicly by the Sashiko AI review bot on dri-devel on
13 September 2026, in its automated review of my own mailbox register-offset
series, where I then listed it as deferred in the v2 cover letter; this patch
is the measured follow-up to that report.
To be precise about scope, because that report raises two items and this patch
addresses only the first: it bounds the ring's placement and span against the
mapping. It does not bound rb_size from below, so it does NOT fix the second
item in the same report, where a tiny but still power-of-two rb_size (e.g. 2)
makes mailbox_get_ringbuf_size() - sizeof(u32) underflow and widens the
send-path wrap bound. I checked that case against this patch and the patch
accepts it: is_power_of_2(2) passes, and all four of the added clauses read
false on an honest rb_start_addr. That item is not mine to claim, it was
published in the same message, and if it is addressed it should be a separate
patch crediting the same report. I have not measured it; the underflow is on
the x2i/send side and the harness described below has no x2i override.
The mailbox ring start address and size are both supplied by firmware, and
rb_start_addr is used directly as an offset into the ioremapped SRAM BAR
without being compared against the length of that mapping. The only gate
between the firmware words and the first readl() on the receive path is
is_power_of_2(rb_size) in xdna_mailbox_start_channel(); the three bounds
tests inside mailbox_get_msg() are all relative to the firmware-supplied
rb_size rather than to the mapped region, and struct
xdna_mailbox_res::ringbuf_size, which the driver fills from
pci_resource_len() and which holds the true window length, is never read
anywhere in the driver. A firmware-supplied i2x_buf that places the receive
ring base past the end of the window therefore produces an out-of-window
read. This patch adds the missing comparison in
xdna_mailbox_start_channel(), which is the single point all three producers
of this geometry pass through (the aie2 management block, the aie2
CREATE_CONTEXT response, and the aie4 mailbox info block), and fails channel
start when the ring does not lie entirely inside the mapping.
How this was measured. On an npu4 device (0000:06:00.1, 1022:17f0 rev 0x10,
firmware amdnpu/17f0_10/npu_7.sbin, fw_version 1.1.2.64, HP OMEN 16-ap0xxx,
kernel 7.1.5) I built the driver out of tree with instrumentation that logs
the ring geometry and recomputes, for every ring access, whether the target
lies inside res.ringbuf_base + res.ringbuf_size. A module parameter
overrides exactly one firmware field, info_regs.i2x_buf, at the point it
lands in aie2_get_mgmt_chann_info(), i.e. upstream of every gate, so the
whole downstream path is the driver's real one. The override moved the
receive ring base to BAR2 offset 0x80000, one byte past a 0x80000 window.
The channel started with no complaint, the only check applied being
is_power_of_2(rb_size)=1, and the first response from firmware then drove
mailbox_get_msg(). The peek readl() faulted:
BUG: unable to handle page fault for address: ffffd0db86b80000
#PF: supervisor read access in kernel mode
#PF: error_code(0x0000) - not-present page
Workqueue: xdna_mailbox mailbox_rx_worker [amdxdna]
RIP: 0010:mailbox_rx_worker.cold+0xb6/0x43b [amdxdna]
The faulting address is exactly res.ringbuf_base + res.ringbuf_size in both
runs; pcim_iomap(pdev, 2, 0) maps the whole BAR, so one byte past the window
is already past the mapping and the PTE is zero. This happens during probe,
before /dev/accel/accel0 is created, so no userspace component is involved.
As a secondary effect the oops kills the mailbox worker mid work item, and
the RX timeout path then calls drain_workqueue() on a work item whose worker
is dead, so probe wedges in uninterruptible sleep and the module cannot be
unloaded.
The same override was run twice, from two independent boots, the second with
preconditions recorded beforehand (tainted 0, in-tree module, device bound).
Everything that identifies the defect was identical across the two runs: the
access type, the not-present PTE, the workqueue, the RIP symbol and offset,
the faulting instruction bytes, every frame and offset of the call trace, and
fault minus base equal to res.ringbuf_size. The only differences are the
ioremap base and the module load base, which a reboot is expected to move.
A benign control, the unmodified firmware geometry, was run through the same
instrumented build across two runs, with both the straight-line and the wrap
branch of the send path exercised, and all 513 bounded ring accesses landed
inside the window.
What I am not claiming. Both injection arms overrode the i2x (device to
host) direction only and left x2i untouched, so the send path ran on honest
geometry throughout. There is no measurement here of what an out-of-window
x2i->rb_start_addr would do at the memcpy_toio() in mailbox_send_msg(); no
write-path injection was attempted, and nothing in this report should be read
as a write-side or code-execution claim. What was measured is a 4-byte
out-of-window read that faults on a not-present page, and the probe wedge
that follows. The per-hwctx path is also uncovered: the second producer of
these same values is the CREATE_CONTEXT response in aie2_message.c, and
exercising it needs a userspace stack to create a hardware context, which
was not available on this box, so only the management channel was driven.
The instrumentation for the cq_pair path exists and never fired. The same
absence of a window check applies there on paper, and the fix sits in the
common function, but that path was not measured. Separately, a second arm
that overrode rb_size instead of rb_start_addr was accepted by the driver
and recorded as exceeding the window, yet produced no out-of-bounds access
in 137 accesses, so rb_size alone is not sufficient to leave the mapping;
rb_start_addr is.
The patched driver was then run on the same device, six arms across six clean
boots, each arm a single override of the same instrumented build (one module,
one sha256, no rebuild between arms). Honest geometry is accepted and the
device works normally: probe completes, /dev/accel/accel0 appears, 10 workload
passes return 100/100 queries, and 32 real ring accesses all land in window.
Four out-of-bounds geometries are rejected at channel start, before any ring
access, with no page fault, no oops, and a clean module unload in about two
seconds: rb_start_addr equal to the window length, rb_start_addr strictly
greater than it, an oversized span, and a non-power-of-two rb_size handled by
the gate already shipped. The rejection cascade is byte-identical across those
arms except for which gate names itself.
What that does and does not establish, clause by clause. The two receive-side
clauses are each exercised in isolation: the span clause by an oversized
rb_size on an honest rb_start_addr, and the placement clause by
rb_start_addr=0x100000 against a measured ringbuf_size=0x80000, where the span
clause cannot fire at all because ringbuf_size - rb_start_addr underflows in
size_t to 0xFFFFFFFFFFF80000. That is the arm which shows the two clauses are
not redundant. Two limits on it, stated rather than left to be inferred: the
kernel short-circuits at the placement clause and so never evaluates the span
clause on that input, which means the span clause's falsity there is
arithmetic over hardware-measured operands rather than an observed branch, and
I did not build a variant with the clause removed to show acceptance directly;
and that arm has had one hardware run, not two, so I am not describing it as
reproduced. The two x2i clauses were never injected at all and remain
unexercised, for the reason given above.
Finally, the injected values stand in for firmware rather than coming from
it. The module parameter writes the word where the firmware word lands, so
the path below that point is real, but the firmware itself was never made to
publish these values. npu*.sbin is signed and loaded from root-owned
/lib/firmware, so this is not reachable by an unprivileged user on a healthy
system; I am proposing it as the same kind of defensive check the file
already applies to other firmware input on this path (the MGMT_MBOX_MAGIC
check, aie2_check_protocol(), and the tail pointer in mailbox_get_msg()).
The full oops from both runs, the instrumented build, the runners, the
control run and the complete measurement record are available on request
rather than pasted here.
Eva Crystal (0xiviel)
XSource Security
https://xsourcesec.com
drivers/accel/amdxdna/amdxdna_mailbox.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index 05c3786de135..896b7503cf8d 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -518,6 +518,19 @@ xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
return -EINVAL;
}
+ /*
+ * The ring start address and size are supplied by firmware and are
+ * used as an offset into the mapped ring buffer region. Check that
+ * the ring described lies entirely inside it.
+ */
+ if (x2i->rb_start_addr >= mb_chann->mb->res.ringbuf_size ||
+ x2i->rb_size > mb_chann->mb->res.ringbuf_size - x2i->rb_start_addr ||
+ i2x->rb_start_addr >= mb_chann->mb->res.ringbuf_size ||
+ i2x->rb_size > mb_chann->mb->res.ringbuf_size - i2x->rb_start_addr) {
+ pr_err("Ring buf does not fit in the mapped region\n");
+ 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] accel/amdxdna: bound the firmware-supplied mailbox ring against the mapped region
2026-10-02 6:37 [PATCH] accel/amdxdna: bound the firmware-supplied mailbox ring against the mapped region Eva Crystal
@ 2026-10-02 16:34 ` Lizhi Hou
2026-10-02 20:58 ` [PATCH] accel/amdxdna: document trusted mailbox ring geometry and drop the power-of-two check Eva Crystal
0 siblings, 1 reply; 3+ messages in thread
From: Lizhi Hou @ 2026-10-02 16:34 UTC (permalink / raw)
To: Eva Crystal, Min Ma, Oded Gabbay
Cc: dri-devel, linux-kernel, sashiko-reviews, Sashiko AI review
On 10/1/26 23:37, Eva Crystal wrote:
> The mailbox ring start address and size both come from firmware, and
> rb_start_addr is used directly as an offset into the ioremapped SRAM BAR:
>
> read_addr = mb_chann->mb->res.ringbuf_base + start_addr + head;
> header.total_size = readl(read_addr);
>
> Nothing compares it against the length of that mapping. The only gate
> between the firmware words and that readl() is is_power_of_2(rb_size) in
> xdna_mailbox_start_channel(); the bounds tests in mailbox_get_msg() are
> all relative to the firmware-supplied rb_size, not to the mapping. The
> driver does record the true length, in struct
> xdna_mailbox_res::ringbuf_size from pci_resource_len(), but that field is
> never read anywhere.
>
> So firmware can describe a ring that starts past the end of the BAR, and
> the receive path reads there. Measured on an npu4 device (0000:06:00.1,
> 1022:17f0 rev 0x10, fw 1.1.2.64) by moving i2x_buf one page beyond the
> window: the channel starts, the first firmware response drives
> mailbox_get_msg(), and the peek readl() in mailbox_rx_worker faults on a
> not-present page at exactly ringbuf_base + ringbuf_size. pcim_iomap()
> maps the whole BAR, so one byte past the window is already past the
> mapping. This happens during probe, before any device node exists, so no
> userspace is involved. The oops kills the mailbox worker mid work item
> and probe then wedges in drain_workqueue(). Only the read direction was
> measured; nothing is claimed here about the write path.
>
> Reject a channel whose ring does not lie entirely inside the mapping.
> The check sits in xdna_mailbox_start_channel(), which is where all three
> producers of this geometry converge: the management information block in
> aie2_get_mgmt_chann_info(), the CREATE_CONTEXT response in
> aie2_create_context(), and the mailbox information block in
> aie4_mailbox_start().
>
> Reaching this needs firmware that reports geometry it should not.
Thanks for providing the patch. The firmware is signed by AMD and driver
can trust the register reading and management channel responses.
Adding more and more checks to verify the trusted firmware responses
does not seem to be helpful. Instead, I would suggest to add comment for
this which helps sashiko to better understand the design.
There are few legacy checks for rb_size which was used for firmware
debugging and those should be removed.
Lizhi
> npu*.sbin is signed and loaded from root-owned /lib/firmware, so this is
> not something an unprivileged user can drive. The file already validates
> other firmware input on this path, the MGMT_MBOX_MAGIC check,
> aie2_check_protocol() and the tail pointer in mailbox_get_msg(), so the
> standard applied here is the one it already sets for itself.
>
> Reported-by: Sashiko AI review <sashiko-bot@kernel.org>
> Link: https://lore.kernel.org/all/20260913214451.639251F000FF@smtp.kernel.org/
> Signed-off-by: Eva Crystal <0xiviel@gmail.com>
> ---
>
> This was first raised publicly by the Sashiko AI review bot on dri-devel on
> 13 September 2026, in its automated review of my own mailbox register-offset
> series, where I then listed it as deferred in the v2 cover letter; this patch
> is the measured follow-up to that report.
>
> To be precise about scope, because that report raises two items and this patch
> addresses only the first: it bounds the ring's placement and span against the
> mapping. It does not bound rb_size from below, so it does NOT fix the second
> item in the same report, where a tiny but still power-of-two rb_size (e.g. 2)
> makes mailbox_get_ringbuf_size() - sizeof(u32) underflow and widens the
> send-path wrap bound. I checked that case against this patch and the patch
> accepts it: is_power_of_2(2) passes, and all four of the added clauses read
> false on an honest rb_start_addr. That item is not mine to claim, it was
> published in the same message, and if it is addressed it should be a separate
> patch crediting the same report. I have not measured it; the underflow is on
> the x2i/send side and the harness described below has no x2i override.
>
> The mailbox ring start address and size are both supplied by firmware, and
> rb_start_addr is used directly as an offset into the ioremapped SRAM BAR
> without being compared against the length of that mapping. The only gate
> between the firmware words and the first readl() on the receive path is
> is_power_of_2(rb_size) in xdna_mailbox_start_channel(); the three bounds
> tests inside mailbox_get_msg() are all relative to the firmware-supplied
> rb_size rather than to the mapped region, and struct
> xdna_mailbox_res::ringbuf_size, which the driver fills from
> pci_resource_len() and which holds the true window length, is never read
> anywhere in the driver. A firmware-supplied i2x_buf that places the receive
> ring base past the end of the window therefore produces an out-of-window
> read. This patch adds the missing comparison in
> xdna_mailbox_start_channel(), which is the single point all three producers
> of this geometry pass through (the aie2 management block, the aie2
> CREATE_CONTEXT response, and the aie4 mailbox info block), and fails channel
> start when the ring does not lie entirely inside the mapping.
>
> How this was measured. On an npu4 device (0000:06:00.1, 1022:17f0 rev 0x10,
> firmware amdnpu/17f0_10/npu_7.sbin, fw_version 1.1.2.64, HP OMEN 16-ap0xxx,
> kernel 7.1.5) I built the driver out of tree with instrumentation that logs
> the ring geometry and recomputes, for every ring access, whether the target
> lies inside res.ringbuf_base + res.ringbuf_size. A module parameter
> overrides exactly one firmware field, info_regs.i2x_buf, at the point it
> lands in aie2_get_mgmt_chann_info(), i.e. upstream of every gate, so the
> whole downstream path is the driver's real one. The override moved the
> receive ring base to BAR2 offset 0x80000, one byte past a 0x80000 window.
> The channel started with no complaint, the only check applied being
> is_power_of_2(rb_size)=1, and the first response from firmware then drove
> mailbox_get_msg(). The peek readl() faulted:
>
> BUG: unable to handle page fault for address: ffffd0db86b80000
> #PF: supervisor read access in kernel mode
> #PF: error_code(0x0000) - not-present page
> Workqueue: xdna_mailbox mailbox_rx_worker [amdxdna]
> RIP: 0010:mailbox_rx_worker.cold+0xb6/0x43b [amdxdna]
>
> The faulting address is exactly res.ringbuf_base + res.ringbuf_size in both
> runs; pcim_iomap(pdev, 2, 0) maps the whole BAR, so one byte past the window
> is already past the mapping and the PTE is zero. This happens during probe,
> before /dev/accel/accel0 is created, so no userspace component is involved.
> As a secondary effect the oops kills the mailbox worker mid work item, and
> the RX timeout path then calls drain_workqueue() on a work item whose worker
> is dead, so probe wedges in uninterruptible sleep and the module cannot be
> unloaded.
>
> The same override was run twice, from two independent boots, the second with
> preconditions recorded beforehand (tainted 0, in-tree module, device bound).
> Everything that identifies the defect was identical across the two runs: the
> access type, the not-present PTE, the workqueue, the RIP symbol and offset,
> the faulting instruction bytes, every frame and offset of the call trace, and
> fault minus base equal to res.ringbuf_size. The only differences are the
> ioremap base and the module load base, which a reboot is expected to move.
> A benign control, the unmodified firmware geometry, was run through the same
> instrumented build across two runs, with both the straight-line and the wrap
> branch of the send path exercised, and all 513 bounded ring accesses landed
> inside the window.
>
> What I am not claiming. Both injection arms overrode the i2x (device to
> host) direction only and left x2i untouched, so the send path ran on honest
> geometry throughout. There is no measurement here of what an out-of-window
> x2i->rb_start_addr would do at the memcpy_toio() in mailbox_send_msg(); no
> write-path injection was attempted, and nothing in this report should be read
> as a write-side or code-execution claim. What was measured is a 4-byte
> out-of-window read that faults on a not-present page, and the probe wedge
> that follows. The per-hwctx path is also uncovered: the second producer of
> these same values is the CREATE_CONTEXT response in aie2_message.c, and
> exercising it needs a userspace stack to create a hardware context, which
> was not available on this box, so only the management channel was driven.
> The instrumentation for the cq_pair path exists and never fired. The same
> absence of a window check applies there on paper, and the fix sits in the
> common function, but that path was not measured. Separately, a second arm
> that overrode rb_size instead of rb_start_addr was accepted by the driver
> and recorded as exceeding the window, yet produced no out-of-bounds access
> in 137 accesses, so rb_size alone is not sufficient to leave the mapping;
> rb_start_addr is.
>
> The patched driver was then run on the same device, six arms across six clean
> boots, each arm a single override of the same instrumented build (one module,
> one sha256, no rebuild between arms). Honest geometry is accepted and the
> device works normally: probe completes, /dev/accel/accel0 appears, 10 workload
> passes return 100/100 queries, and 32 real ring accesses all land in window.
> Four out-of-bounds geometries are rejected at channel start, before any ring
> access, with no page fault, no oops, and a clean module unload in about two
> seconds: rb_start_addr equal to the window length, rb_start_addr strictly
> greater than it, an oversized span, and a non-power-of-two rb_size handled by
> the gate already shipped. The rejection cascade is byte-identical across those
> arms except for which gate names itself.
>
> What that does and does not establish, clause by clause. The two receive-side
> clauses are each exercised in isolation: the span clause by an oversized
> rb_size on an honest rb_start_addr, and the placement clause by
> rb_start_addr=0x100000 against a measured ringbuf_size=0x80000, where the span
> clause cannot fire at all because ringbuf_size - rb_start_addr underflows in
> size_t to 0xFFFFFFFFFFF80000. That is the arm which shows the two clauses are
> not redundant. Two limits on it, stated rather than left to be inferred: the
> kernel short-circuits at the placement clause and so never evaluates the span
> clause on that input, which means the span clause's falsity there is
> arithmetic over hardware-measured operands rather than an observed branch, and
> I did not build a variant with the clause removed to show acceptance directly;
> and that arm has had one hardware run, not two, so I am not describing it as
> reproduced. The two x2i clauses were never injected at all and remain
> unexercised, for the reason given above.
>
> Finally, the injected values stand in for firmware rather than coming from
> it. The module parameter writes the word where the firmware word lands, so
> the path below that point is real, but the firmware itself was never made to
> publish these values. npu*.sbin is signed and loaded from root-owned
> /lib/firmware, so this is not reachable by an unprivileged user on a healthy
> system; I am proposing it as the same kind of defensive check the file
> already applies to other firmware input on this path (the MGMT_MBOX_MAGIC
> check, aie2_check_protocol(), and the tail pointer in mailbox_get_msg()).
>
> The full oops from both runs, the instrumented build, the runners, the
> control run and the complete measurement record are available on request
> rather than pasted here.
>
> Eva Crystal (0xiviel)
> XSource Security
> https://xsourcesec.com
>
> drivers/accel/amdxdna/amdxdna_mailbox.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
> index 05c3786de135..896b7503cf8d 100644
> --- a/drivers/accel/amdxdna/amdxdna_mailbox.c
> +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
> @@ -518,6 +518,19 @@ xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
> return -EINVAL;
> }
>
> + /*
> + * The ring start address and size are supplied by firmware and are
> + * used as an offset into the mapped ring buffer region. Check that
> + * the ring described lies entirely inside it.
> + */
> + if (x2i->rb_start_addr >= mb_chann->mb->res.ringbuf_size ||
> + x2i->rb_size > mb_chann->mb->res.ringbuf_size - x2i->rb_start_addr ||
> + i2x->rb_start_addr >= mb_chann->mb->res.ringbuf_size ||
> + i2x->rb_size > mb_chann->mb->res.ringbuf_size - i2x->rb_start_addr) {
> + pr_err("Ring buf does not fit in the mapped region\n");
> + 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));
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] accel/amdxdna: document trusted mailbox ring geometry and drop the power-of-two check
2026-10-02 16:34 ` Lizhi Hou
@ 2026-10-02 20:58 ` Eva Crystal
0 siblings, 0 replies; 3+ messages in thread
From: Eva Crystal @ 2026-10-02 20:58 UTC (permalink / raw)
To: Lizhi Hou, Min Ma, Oded Gabbay
Cc: Max Zhen, dri-devel, linux-kernel, sashiko-reviews, sashiko-bot
The mailbox ring buffer geometry comes from AMD signed firmware, through
the management or mailbox information block, or through the CREATE_CONTEXT
response, and the driver trusts it by design.
The power-of-two test on rb_size was a firmware debugging aid. Nothing in
the driver derives a mask, a shift or a modulo from rb_size, so no code
depends on the property it tested. The ring index wrap is handled by the
explicit comparisons in mailbox_send_msg() and mailbox_get_msg().
Replace the test with a comment that records the design, so that
automated review does not flag the absence of a bound here again.
Suggested-by: Lizhi Hou <lizhi.hou@amd.com>
Link: https://lore.kernel.org/all/67698952-6394-44a8-01fe-3e3558b7cb4c@amd.com/
Signed-off-by: Eva Crystal <0xiviel@gmail.com>
---
The pkg_size check in xdna_mailbox_send_msg() is left in place on
purpose: it bounds a driver-supplied size, and with Max Zhen's pool
patch (20261002161122.1350075-1-max.zhen@amd.com) it is the only bound
on the write into a pre-allocated message slot.
drivers/accel/amdxdna/amdxdna_mailbox.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index 05c3786de135..c4a36858668f 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -513,11 +513,13 @@ xdna_mailbox_start_channel(struct mailbox_channel *mb_chann,
{
int ret;
- if (!is_power_of_2(x2i->rb_size) || !is_power_of_2(i2x->rb_size)) {
- pr_err("Ring buf size must be power of 2\n");
- return -EINVAL;
- }
-
+ /*
+ * The ring buffer geometry, rb_start_addr and rb_size for both the
+ * x2i and the i2x channel, comes from AMD signed firmware, through
+ * the management or mailbox information block, or through the
+ * CREATE_CONTEXT response. The driver trusts those values by design
+ * and does not bound them against the mapped region.
+ */
mb_chann->msix_irq = mb_irq;
mb_chann->iohub_int_addr = iohub_int_addr;
memcpy(&mb_chann->res[CHAN_RES_X2I], x2i, sizeof(*x2i));
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-02 21:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-02 6:37 [PATCH] accel/amdxdna: bound the firmware-supplied mailbox ring against the mapped region Eva Crystal
2026-10-02 16:34 ` Lizhi Hou
2026-10-02 20:58 ` [PATCH] accel/amdxdna: document trusted mailbox ring geometry and drop the power-of-two check Eva Crystal
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®