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

[sashiko-bot@kernel.org]

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

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/cover.1789334558.git.0xiviel@gmail.com?part=2