Add a prefix on Subject: please. Same for previous in series.
On Mon, 8 Nov 2021 16:46:33 -0800 John Johnson <john.g.john...@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.john...@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, > +};