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

[Bin Meng <bmeng.cn@gmail.com>]

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