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

[John Johnson]

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

ok

> On Mon,  8 Nov 2021 16:46:33 -0800
> John Johnson  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 
>> ---
>> 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 

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

[Alex Williamson]

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

On Mon,  8 Nov 2021 16:46:33 -0800
John Johnson  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 
> ---
>  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 = 

[RFC v3 05/19] Add validation ops vector

[John Johnson]

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 
---
 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;
+}
+/* account for extra migration region */
+if (info->num_regions > VFIO_PCI_NUM_REGIONS + 1) {
+return -EINVAL;
+}
+if (info->num_irqs > VFIO_PCI_NUM_IRQS) {
+return -EINVAL;
+}
+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;
+}
+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;
+}
+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,
+};
-- 
1.8.3.1