Re: [PATCH 07/11] virtio: pci: Check virtio configs are mapped

[Andrew Scull <ascull@google.com>]

On Fri, 25 Mar 2022 at 04:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull@google.com> wrote:
> >
> > The calls to `virtio_pci_map_capability()` return NULL on error. If this
> > happens, later accesses to the pointers would be unsafe. Avoid this by
> > failing if the configs were not successfully mapped.
> >
> > Signed-off-by: Andrew Scull <ascull@google.com>
> > ---
> >  drivers/virtio/virtio_pci_modern.c | 25 ++++++++++++++++++++-----
> >  1 file changed, 20 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> > index 38a0da0a84..2f1a1cedbc 100644
> > --- a/drivers/virtio/virtio_pci_modern.c
> > +++ b/drivers/virtio/virtio_pci_modern.c
> > @@ -534,7 +534,19 @@ static int virtio_pci_probe(struct udevice *udev)
> >                 return -EINVAL;
> >         }
> >
> > +       /* Map configuration structures */
> > +       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > +       if (!priv->common) {
>
> This can't be NULL, as you did not pass a NULL capability pointer
>
> > +               printf("(%s): could not map common config\n", udev->name);
> > +               return -EINVAL;
> > +       }
> > +
> >         priv->notify_len = notify_cap.length;
> > +       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > +       if (!priv->notify_base) {
> > +               printf("(%s): could not map notify config\n", udev->name);
> > +               return -EINVAL;
> > +       }
> >
> >         /*
> >          * Device capability is only mandatory for devices that have
> > @@ -543,13 +555,16 @@ static int virtio_pci_probe(struct udevice *udev)
> >         device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> >                                             sizeof(struct virtio_pci_cap),
> >                                             &device_cap);
> > -       if (device)
> > +       if (device) {
> >                 priv->device_len = device_cap.length;
> > +               priv->device = virtio_pci_map_capability(udev, &device_cap);
> > +               if (!priv->device) {
> > +                       printf("(%s): could not map device config\n",
> > +                              udev->name);
> > +                       return -EINVAL;
> > +               }
> > +       }
> >
> > -       /* Map configuration structures */
> > -       priv->common = virtio_pci_map_capability(udev, &common_cap);
> > -       priv->notify_base = virtio_pci_map_capability(udev, &notify_cap);
> > -       priv->device = virtio_pci_map_capability(udev, &device_cap);
> >         debug("(%p): common @ %p, notify base @ %p, device @ %p\n",
> >               udev, priv->common, priv->notify_base, priv->device);
> >
>
> I don't think adding these checks is necessary.

See later patches in the series that validate the address range is
within the declared PCI ranges and not an arbitrary range chosen,
accidently or maliciously, by the device. If those checks fail, the
memory will not have been mapped and the probe should fail.

> Regards,
> Bin