[PATCH 05/11] virtio: pci: Check virtio capability is in bounds

[Bin Meng]

On Sun, Mar 20, 2022 at 7:41 PM Andrew Scull <ascull at google.com> wrote:
>
> Ensure the virtio PCI capabilities are contained within the bounds of
> the device's configuration space. The expected size of the capability is
> passed when searching for the capability to enforce this check.
>
> 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 3403ff5cca..4b346be257 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -392,18 +392,30 @@ static int virtio_pci_notify(struct udevice *udev, struct virtqueue *vq)
>   *
>   * @udev:      the transport device
>   * @cfg_type:  the VIRTIO_PCI_CAP_* value we seek
> + * @cap_size:  expected size of the capability
>   *
>   * Return: offset of the configuration structure
>   */
> -static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type)
> +static int virtio_pci_find_capability(struct udevice *udev, u8 cfg_type,
> +                                     size_t cap_size)
>  {
>         int pos;
>         int offset;
>         u8 type, bar;
>
> +       if (cap_size < sizeof(struct virtio_pci_cap))
> +               return 0;
> +
> +       if (cap_size > PCI_CFG_SPACE_SIZE)
> +               return 0;
> +

The above 2 checks are not necessary as this helper is local to this
driver, and we know the callers do things correctly.

>         for (pos = dm_pci_find_capability(udev, PCI_CAP_ID_VNDR);
>              pos > 0;
>              pos = dm_pci_find_next_capability(udev, pos, PCI_CAP_ID_VNDR)) {
> +               /* Ensure the capability is within bounds */
> +               if (PCI_CFG_SPACE_SIZE - cap_size < pos)
> +                       return 0;
> +
>                 offset = pos + offsetof(struct virtio_pci_cap, cfg_type);
>                 dm_pci_read_config8(udev, offset, &type);
>                 offset = pos + offsetof(struct virtio_pci_cap, bar);
> @@ -491,7 +503,8 @@ static int virtio_pci_probe(struct udevice *udev)
>         uc_priv->vendor = subvendor;
>
>         /* Check for a common config: if not, use legacy mode (bar 0) */
> -       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG);
> +       common = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_COMMON_CFG,
> +                                           sizeof(struct virtio_pci_cap));
>         if (!common) {
>                 printf("(%s): leaving for legacy driver\n", udev->name);
>                 return -ENODEV;
> @@ -505,7 +518,8 @@ static int virtio_pci_probe(struct udevice *udev)
>         }
>
>         /* If common is there, notify should be too */
> -       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG);
> +       notify = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_NOTIFY_CFG,
> +                                           sizeof(struct virtio_pci_notify_cap));
>         if (!notify) {
>                 printf("(%s): missing capabilities %i/%i\n", udev->name,
>                        common, notify);
> @@ -519,7 +533,8 @@ static int virtio_pci_probe(struct udevice *udev)
>          * Device capability is only mandatory for devices that have
>          * device-specific configuration.
>          */
> -       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG);
> +       device = virtio_pci_find_capability(udev, VIRTIO_PCI_CAP_DEVICE_CFG,
> +                                           sizeof(struct virtio_pci_cap));
>         if (device) {
>                 offset = device + offsetof(struct virtio_pci_cap, length);
>                 dm_pci_read_config32(udev, offset, &priv->device_len);
> --

Regards,
Bin