Re: [RFC v3 05/19] Add validation ops vector

[Alex Williamson <alex.williamson@redhat.com>]

Add a prefix on Subject: please.  Same for previous in series.

On Mon,  8 Nov 2021 16:46:33 -0800
John Johnson <john.g.johnson@oracle.com> wrote:

> Validates cases where the return values aren't fully trusted
> (prep work for vfio-user, where the return values from the
> remote process aren't trusted)
> 
> Signed-off-by: John G Johnson <john.g.johnson@oracle.com>
> ---
>  include/hw/vfio/vfio-common.h | 21 ++++++++++++++
>  hw/vfio/pci.c                 | 67 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 88 insertions(+)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 43fa948..c0dbbfb 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -125,6 +125,7 @@ typedef struct VFIOHostDMAWindow {
>  
>  typedef struct VFIODeviceOps VFIODeviceOps;
>  typedef struct VFIODevIO VFIODevIO;
> +typedef struct VFIOValidOps VFIOValidOps;
>  
>  typedef struct VFIODevice {
>      QLIST_ENTRY(VFIODevice) next;
> @@ -141,6 +142,7 @@ typedef struct VFIODevice {
>      bool enable_migration;
>      VFIODeviceOps *ops;
>      VFIODevIO *io_ops;
> +    VFIOValidOps *valid_ops;
>      unsigned int num_irqs;
>      unsigned int num_regions;
>      unsigned int flags;
> @@ -214,6 +216,25 @@ struct VFIOContIO {
>  extern VFIODevIO vfio_dev_io_ioctl;
>  extern VFIOContIO vfio_cont_io_ioctl;
>  
> +/*
> + * This ops vector allows for bus-specific verification
> + * routines in cases where the server may not be fully
> + * trusted.
> + */
> +struct VFIOValidOps {
> +    int (*validate_get_info)(VFIODevice *vdev, struct vfio_device_info *info);
> +    int (*validate_get_region_info)(VFIODevice *vdev,
> +                                    struct vfio_region_info *info, int *fd);
> +    int (*validate_get_irq_info)(VFIODevice *vdev, struct vfio_irq_info *info);
> +};
> +
> +#define VDEV_VALID_INFO(vdev, info) \
> +    ((vdev)->valid_ops->validate_get_info((vdev), (info)))
> +#define VDEV_VALID_REGION_INFO(vdev, info, fd) \
> +    ((vdev)->valid_ops->validate_get_region_info((vdev), (info), (fd)))
> +#define VDEV_VALID_IRQ_INFO(vdev, irq) \
> +    ((vdev)->valid_ops->validate_get_irq_info((vdev), (irq)))
> +
>  #endif /* CONFIG_LINUX */
>  
>  typedef struct VFIOGroup {
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 28f21f8..6e2ce35 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3371,3 +3371,70 @@ static void register_vfio_pci_dev_type(void)
>  }
>  
>  type_init(register_vfio_pci_dev_type)
> +
> +
> +/*
> + * PCI validation ops - used when return values need
> + * validation before use
> + */
> +
> +static int vfio_pci_valid_info(VFIODevice *vbasedev,
> +                               struct vfio_device_info *info)
> +{
> +    /* must be PCI */
> +    if ((info->flags & VFIO_DEVICE_FLAGS_PCI) == 0) {
> +        return -EINVAL;
> +    }
> +    /* only other valid flag is reset */
> +    if (info->flags & ~(VFIO_DEVICE_FLAGS_PCI | VFIO_DEVICE_FLAGS_RESET)) {
> +        return -EINVAL;
> +    }

This means QEMU vfio-pci breaks on any extension of the flags field.

> +    /* account for extra migration region */
> +    if (info->num_regions > VFIO_PCI_NUM_REGIONS + 1) {
> +        return -EINVAL;
> +    }

This is also invalid, there can be device specific regions beyond
migration.

> +    if (info->num_irqs > VFIO_PCI_NUM_IRQS) {
> +        return -EINVAL;
> +    }

And device specific IRQs.

> +    return 0;
> +}
> +
> +static int vfio_pci_valid_region_info(VFIODevice *vbasedev,
> +                                          struct vfio_region_info *info,
> +                                          int *fd)
> +{
> +    if (info->flags & ~(VFIO_REGION_INFO_FLAG_READ |
> +                        VFIO_REGION_INFO_FLAG_WRITE |
> +                        VFIO_REGION_INFO_FLAG_MMAP |
> +                        VFIO_REGION_INFO_FLAG_CAPS)) {
> +        return -EINVAL;
> +    }

Similarly, this allows zero future extensions.  Notice for instance how
the CAPS flag was added later as a backwards compatible extension.

> +    if (info->index > vbasedev->num_regions) {
> +        return -EINVAL;
> +    }
> +    /* cap_offset in valid area */
> +    if ((info->flags & VFIO_REGION_INFO_FLAG_CAPS) &&
> +        (info->cap_offset < sizeof(*info) || info->cap_offset > info->argsz)) {
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +static int vfio_pci_valid_irq_info(VFIODevice *vbasedev,
> +                                 struct vfio_irq_info *info)
> +{
> +    if (info->flags & ~(VFIO_IRQ_INFO_EVENTFD | VFIO_IRQ_INFO_MASKABLE |
> +                        VFIO_IRQ_INFO_AUTOMASKED | VFIO_IRQ_INFO_NORESIZE)) {
> +        return -EINVAL;
> +    }

Similarly, nak.  Thanks,

Alex

> +    if (info->index > vbasedev->num_irqs) {
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
> +struct VFIOValidOps vfio_pci_valid_ops = {
> +    .validate_get_info = vfio_pci_valid_info,
> +    .validate_get_region_info = vfio_pci_valid_region_info,
> +    .validate_get_irq_info = vfio_pci_valid_irq_info,
> +};