Re: [Qemu-devel] [RFC PATCH 2/4] hw/vfio/common.c : vfio_get_dev_property

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

On Mon, 2015-01-12 at 14:21 +0100, Baptiste Reynal wrote:
> Add a function to handle ioctl VFIO_DEVICE_GET_DEV_PROPERTY
> to retrieve properties from a VFIO device.
> 
> Signed-off-by: Baptiste Reynal <b.reynal@virtualopensystems.com>
> ---
>  hw/vfio/common.c              | 33 +++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio-common.h |  2 ++
>  2 files changed, 35 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index ba00ec9..698d2c4 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -958,3 +958,36 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>  
>      return vfio_container_do_ioctl(as, groupid, req, param);
>  }
> +
> +struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
> +        unsigned int type)
> +{
> +    unsigned int length = 0;
> +    struct vfio_dev_property *property = NULL;
> +    int ret;
> +
> +    length = strlen(name) + 1;
> +
> +    while (1) {
> +        unsigned int argsz = sizeof(struct vfio_dev_property) + length;
> +        property = realloc(property, argsz);

By my read, realloc() doesn't give zero'd memory, so property->length is
uninitialized here.

> +        property->argsz = argsz;
> +        property->type = type;
> +        strcpy((char *) property->data, name);
> +
> +        ret = ioctl(device, VFIO_DEVICE_GET_DEV_PROPERTY, property);

This ioctl might not exit.

> +        if (length < property->length) {

Which means this compares length to random memory and potentially causes
a segfault when trying to realloc.

What types of devices are going to have VFIO_DEVICE_GET_DEV_PROPERTY and
is this appropriate for common?  The error and return here leaves
something to be desired.  Maybe only return for a given error.

> +            length = property->length;
> +        } else {
> +            break;
> +        }
> +    }
> +
> +    if (ret) {
> +        g_free(property);
> +        property = NULL;
> +    }
> +
> +    return property;
> +}
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index 2f1b09c..9c649cd 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -149,6 +149,8 @@ VFIOGroup *vfio_get_group(int groupid, AddressSpace *as);
>  void vfio_put_group(VFIOGroup *group);
>  int vfio_get_device(VFIOGroup *group, const char *name,
>                      VFIODevice *vbasedev);
> +struct vfio_dev_property *vfio_get_dev_property(int device, const char *name,
> +        unsigned int type);
>  
>  extern const MemoryRegionOps vfio_region_ops;
>  extern const MemoryListener vfio_memory_listener;