Re: [PATCH 03/11] virtio: pci: Bounds check notification writes

Bin Meng
2022-03-24T14:19:18Z

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull@google.com> wrote:
>
> Make sure virtio notifications are written within their allocated
> buffer.
>
> Signed-off-by: Andrew Scull <ascull@google.com>
> ---
> drivers/virtio/virtio_pci_modern.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index bcf9f18997..60bdc53a6d 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -101,6 +101,7 @@
> struct virtio_pci_priv {
> struct virtio_pci_common_cfg __iomem *common;
> void __iomem *notify_base;
> + u32 notify_len;
> void __iomem *device;
> u32 device_len;
> u32 notify_offset_multiplier;
> @@ -372,12 +373,16 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> /* get offset of notification word for this vq */
> off = ioread16(&priv->common->queue_notify_off);
>
> + /* Check the effective offset is in bounds */
> + off *= priv->notify_offset_multiplier;
> + if (off > priv->notify_len - sizeof(u16))

This check may not work for devices offering VIRTIO_F_NOTIFICATION_DATA.

> + return -EIO;
> +
> /*
> * We write the queue's selector into the notification register
> * to signal the other end
> */
> - iowrite16(vq->index,
> - priv->notify_base + off * priv->notify_offset_multiplier);
> + iowrite16(vq->index, priv->notify_base + off);
>
> return 0;
> }
> @@ -499,6 +504,9 @@ static int virtio_pci_probe(struct udevice *udev)
> return -EINVAL;
> }
>
> + offset = notify + offsetof(struct virtio_pci_cap, length);
> + dm_pci_read_config32(udev, offset, &priv->notify_len);
> +
> /*
> * Device capability is only mandatory for devices that have
> * device-specific configuration.

Regards,
Bin