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

Bin Meng
2022-03-25T04:38:56Z

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.

Regards,
Bin