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