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

[Bin Meng]

On Fri, Mar 25, 2022 at 3:07 PM Andrew Scull <ascull at google.com> wrote:
>
> On Fri, 25 Mar 2022 at 04:38, Bin Meng <bmeng.cn at gmail.com> wrote:
> >
> > On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull at 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 at 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.

Yep, I see additional checks being added in patch 10, so the patch
order should be adjusted to let this patch come later.

Regards,
Bin