Re: [PATCH V1] accel/amdxdna: eliminate GFP_KERNEL allocation from mailbox send path

[Eva Crystal]

Hi Max,

> +#define MSG_BUF_SZ(chann)                                    \
> +     (mailbox_get_ringbuf_size(chann, CHAN_RES_X2I) +        \
> +      sizeof(struct mailbox_msg))

> +     mb_chann->msg_buf = kcalloc(n_msg, MSG_BUF_SZ(mb_chann), GFP_KERNEL);

rb_size comes from the firmware-provided channel info, and with this
change it now sizes a host allocation as well as the ring itself.
Nothing bounds it before the kcalloc(), so a bad or compromised
firmware value turns directly into an arbitrarily large kernel
allocation. Could rb_size be checked against the ring window it
describes before the pool is allocated?

> +     if (!mb_chann->msg_buf)
> +             return -ENOMEM;

This adds another failure path out of xdna_mailbox_start_channel()
before request_irq(). On failure the caller still holds the freed
channel pointer, which "accel/amdxdna: clear the mailbox channel
pointer when starting it fails" (Reviewed-by: Lizhi Hou) fixes, so the
two patches should be fine in either order.

Thanks,

Eva Crystal (0xiviel)
XSource Security
https://xsourcesec.com