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

[John Johnson]

> On Nov 19, 2021, at 2:42 PM, Alex Williamson <alex.williamson@redhat.com> 
> wrote:
> 
> 
> Add a prefix on Subject: please.  Same for previous in series.
> 

        ok

> 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,
> 


        The verification routines are to sanitize the data returned when we
don’t trust the server.  The kernel driver return values are not verified, as
the kernel has much better ways of corrupting the client.  When new flags
are added, the they’d be added here, too.

        I can push them down into the the vfio-user code, but that forces
the vfio-code to change versions or add capabilities whenever a new flag is
added.

                                                                JJ