Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [High] The firmware-supplied ring buffer offsets (`rb_start_addr`) and sizes (`rb_size`) are not validated against the mapped ring buffer size, leaving the driver vulnerable to out-of-bounds MMIO accesses. - [High] Initial ring buffer head and tail pointers read from firmware are not validated against the ring buffer bounds upon channel start. -- commit e583a0672be522b7766ae36362808346f5791f45 Author: Eva Crystal <[email protected]> accel/amdxdna: bound the firmware-supplied mailbox register offsets This patch adds bounds checking for firmware-supplied mailbox register offsets against the device's mailbox window size. It aims to secure the trust boundary between the driver and firmware by preventing out-of-bounds kernel MMIO accesses through the mailbox mapping. > diff --git a/drivers/accel/amdxdna/amdxdna_mailbox.c > b/drivers/accel/amdxdna/amdxdna_mailbox.c > index cc8865f4e79c9..a390836fe797c 100644 > --- a/drivers/accel/amdxdna/amdxdna_mailbox.c > +++ b/drivers/accel/amdxdna/amdxdna_mailbox.c [ ... ] > @@ -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))) { [Severity: High] This isn't a bug introduced by this patch, but while this secures the mailbox register offsets, does it overlook the ring buffer offsets and sizes? In xdna_mailbox_start_channel(), the firmware-supplied channel resources are checked here, but rb_start_addr and rb_size are not validated against the mapped ringbuf_size. This can lead to out-of-bounds writes later: drivers/accel/amdxdna/amdxdna_mailbox.c:mailbox_send_msg() { ... write_addr = mb_chann->mb->res.ringbuf_base + start_addr + tail; memcpy_toio(write_addr, &mb_msg->pkg, mb_msg->pkg_size); ... } Could a compromised firmware provide maliciously large rb_start_addr or rb_size values, causing out-of-bounds MMIO accesses when calculating write_addr? [Severity: High] This is also a pre-existing issue, but does this function also need to validate the initial head and tail pointers read from the firmware? Right after the new checks in xdna_mailbox_start_channel(), the driver caches the initial pointers directly from the firmware: drivers/accel/amdxdna/amdxdna_mailbox.c:xdna_mailbox_start_channel() { ... mb_chann->x2i_tail = mailbox_get_tailptr(mb_chann, CHAN_RES_X2I); mb_chann->i2x_head = mailbox_get_headptr(mb_chann, CHAN_RES_I2X); ... } If a maliciously large initial tail index is read, could it force an out-of-bounds write in mailbox_send_msg() before normal wrap-around checks occur? drivers/accel/amdxdna/amdxdna_mailbox.c:mailbox_send_msg() { ... write_addr = mb_chann->mb->res.ringbuf_base + start_addr + tail; writel(TOMBSTONE, write_addr); ... } Should we bound check x2i_tail and i2x_head against rb_size during channel initialization? > + 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)); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=2
