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

[Eva Crystal <0xiviel@gmail.com>]

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".

Require 32 bit alignment in the same place. All six users of these
offsets go through mailbox_reg_read() or mailbox_reg_write(), whose
bodies are a bare readl() and writel(); the driver has no narrower or
wider mailbox accessor, so an offset that is not a multiple of four
cannot name a register in this block whatever else is true of it.
mailbox_get_msg() already pairs a range check with IS_ALIGNED(tail, 4)
for the ring tail firmware writes, for the same reason.

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 | 37 +++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c b/drivers/accel/amdxdna/amdxdna_mailbox.c
index cc8865f4e79c..a390836fe797 100644
--- a/drivers/accel/amdxdna/amdxdna_mailbox.c
+++ b/drivers/accel/amdxdna/amdxdna_mailbox.c
@@ -112,6 +112,33 @@ 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 check it against the shape 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 a readl() or a
+	 * writel(), so an offset has to be 32 bit aligned, and leave room for
+	 * 32 bits, to name a register in this mapping at all.
+	 */
+	return IS_ALIGNED(mbox_reg, 4) &&
+	       (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 +545,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 unaligned or outside the %zu byte 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