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

[Bin Meng]

On Fri, Mar 25, 2022 at 12:24 AM Andrew Scull <ascull at google.com> wrote:
>
> On Thu, 24 Mar 2022 at 14:19, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull at google.com> wrote:
> > >
> > > Make sure virtio notifications are written within their allocated
> > > buffer.
> > >
> > > Signed-off-by: Andrew Scull <ascull at 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.
>
> Quickly reading up on VIRTIO_F_NOTIFICATION_DATA (Section 2.7.23 of
> the virtio v1.1, for my reference), it's a feature that can be
> negotiated to allow the driver to pass more details in the
> notification.
>
> However, it isn't negotiated by the u-boot drivers and this is the
> function that generates the notification so we know it's only going to
> write a single 16-bit value. Were VIRTIO_F_NOTIFICATION_DATA support
> to be added, this range check would have to be changed as the size of
> the notification increases.
>
> Does this match your understanding?

Thanks for the pointers. Let's put additional comments there
mentioning that VIRTIO_F_NOTIFICATION_DATA is not used by U-Boot
driver.

>
> > > +               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.

Otherwise,
Reviewed-by: Bin Meng <bmeng.cn at gmail.com>