[PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Bin Meng]

On Fri, Mar 25, 2022 at 12:27 AM Andrew Scull <ascull at google.com> wrote:
>
> On Thu, 24 Mar 2022 at 15:24, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull at google.com> wrote:
> > >
> > > Ensure the virtio PCI capabilities are contained within the bounds of
> > > the device's configuration space. The expected size of the capability is
> > > passed when searching for the capability to enforce this check.
> > >
> > > Signed-off-by: Andrew Scull <ascull at google.com>
> > > ---
> > >  drivers/virtio/virtio_pci_modern.c | 23 +++++++++++++++++++----
> > >  1 file changed, 19 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > > index 3403ff5cca..4b346be257 100644
> > > --- a/drivers/virtio/virtio_pci_modern.c
> > > +++ b/drivers/virtio/virtio_pci_modern.c
> > > @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
> > >   *
> > >   * @udev:      the transport device
> > >   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> > > + * @cap_size:  expected size of the capability
> > >   *
> > >   * Return: offset of the configuration structure
> > >   */
> > > -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> > > +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> > > +                                     size_t cap_size)
> > >  {
> > >         int pos;
> > >         int offset;
> > >         u8 type, bar;
> > >
> > > +       if (cap_size < sizeof(struct virtio_pci_cap))
> > > +               return 0;
> > > +
> > > +       if (cap_size > PCI_CFG_SPACE_SIZE)
> > > +               return 0;
> > > +
> >
> > The above 2 checks are not necessary as this helper is local to this
> > driver, and we know the callers do things correctly.
>
> The checks aren't absolutely necessary but they do help to document
> the constraints and catch any future problems. I'm not sure of the
> philosophy the u-boot applies but I like the idea of the safeguards
> being in place.

I would use assert() for such case.

>
> > >         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
> > >              pos > 0;
> > >              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> > > +               /* Ensure the capability is within bounds */
> > > +               if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> > > +                       return 0;
> > > +
> > >                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
> > >                 dm_pci_read_config8(udev, offset, &type);
> > >                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> > > @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >         uc_priv->vendor = subvendor;
> > >
> > >         /* Check for a common config: if not, use legacy mode (bar 0) */
> > > -       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
> > > +       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> > > +                                           sizeof(struct virtio_pci_cap));
> > >         if (!common) {
> > >                 printf("(%s): leaving for legacy driver\n", udev->name);
> > >                 return -ENODEV;
> > > @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >         }
> > >
> > >         /* If common is there, notify should be too */
> > > -       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
> > > +       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> > > +                                           sizeof(struct virtio_pci_notify_cap));
> > >         if (!notify) {
> > >                 printf("(%s): missing capabilities %i/%i\n", udev->name,
> > >                        common, notify);
> > > @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
> > >          * Device capability is only mandatory for devices that have
> > >          * device-specific configuration.
> > >          */
> > > -       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
> > > +       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> > > +                                           sizeof(struct virtio_pci_cap));
> > >         if (device) {
> > >                 offset = device + offsetof(struct virtio_pci_cap, length);
> > >                 dm_pci_read_config32(udev, offset, &priv->device_len);
> > > --

Regards,
Bin