[PATCH 10/11] virtio: pci: Check mapped range is in a PCI region

[Bin Meng]

On Sun, Mar 20, 2022 at 7:42 PM Andrew Scull <ascull at google.com> wrote:
>
> The virtio PCI capabilities describe a region of memory that should be
> mapped. Ensure those are valid PCI regions before accessing them.
>
> 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 2f1a1cedbc..84750e2b27 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -457,8 +457,9 @@ static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
>  static void __iomem *virtio_pci_map_capability(struct udevice *udev,
>                                                const struct virtio_pci_cap *cap)
>  {
> -       ulong base;
> -       void __iomem *p;
> +       unsigned long mask, flags;
> +       phys_addr_t phys_addr;
> +       u32 base;
>
>         if (!cap)
>                 return NULL;
> @@ -470,9 +471,23 @@ static void __iomem *virtio_pci_map_capability(struct udevice *udev,
>          * For simplicity, only read the BAR address as 32-bit.
>          */
>         base = dm_pci_read_bar32(udev, cap->bar);
> -       p = (void __iomem *)base + cap->offset;
>
> -       return p;
> +       if (U32_MAX - base < cap->offset)
> +               return NULL;
> +       base += cap->offset;
> +
> +       if (U32_MAX - base < cap->length)
> +               return NULL;
> +
> +       /* Find the corresponding memory region that isn't system memory. */
> +       mask = PCI_REGION_TYPE | PCI_REGION_SYS_MEMORY;
> +       flags = PCI_REGION_MEM;
> +       phys_addr = dm_pci_bus_range_to_phys(dev_get_parent(udev), base,
> +                                            cap->length, mask, flags);
> +       if (!phys_addr)
> +               return NULL;
> +
> +       return (void __iomem *)map_physmem(phys_addr, cap->length, MAP_NOCACHE);

Could we use dm_pci_map_bar() instead?

I understand the way the current patch does is to add some sanity
checks against buggy virtio device implementations, but is it really
necessary, or can we move such check somewhere else, like in the
probe()?

>  }
>
>  static int virtio_pci_bind(struct udevice *udev)
> --

Regards,
Bin