Re: [Qemu-devel] [PATCH 6/7] qdev: use int32_t container for devfn property

[Andreas Färber]

Am 27.04.2012 22:21, schrieb Michael Roth:
> Valid range for devfn is -1 to 255 (-1 for automatic assignment). We do
> not currently validate this due to devfn being stored as a uint32_t.
> This can lead to segfaults and other strange behavior.
> 
> We could technically just cast it to int32_t to implement the checking,
> but this will not work for visitor-based setting where we may do additional
> bounds-checking based on target container type, which is int32_t for this
> case.
> 
> Signed-off-by: Michael Roth <address@hidden>

Reviewed-by: Andreas Färber <address@hidden>

Upper limit matches my limited PCI knowledge; cc'ing mst.

/-F

> ---
>  hw/pci.c             |    2 +-
>  hw/pci.h             |    2 +-
>  hw/qdev-properties.c |   11 ++++-------
>  hw/qdev.h            |    2 +-
>  4 files changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index b706e69..7818c9b 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1538,7 +1538,7 @@ PCIDevice *pci_create_multifunction(PCIBus *bus, int 
> devfn, bool multifunction,
>      DeviceState *dev;
>  
>      dev = qdev_create(&bus->qbus, name);
> -    qdev_prop_set_uint32(dev, "addr", devfn);
> +    qdev_prop_set_int32(dev, "addr", devfn);
>      qdev_prop_set_bit(dev, "multifunction", multifunction);
>      return PCI_DEVICE(dev);
>  }
> diff --git a/hw/pci.h b/hw/pci.h
> index 8d0aa49..3bc9218 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -193,7 +193,7 @@ struct PCIDevice {
>  
>      /* the following fields are read only */
>      PCIBus *bus;
> -    uint32_t devfn;
> +    int32_t devfn;
>      char name[64];
>      PCIIORegion io_regions[PCI_NUM_REGIONS];
>  
> diff --git a/hw/qdev-properties.c b/hw/qdev-properties.c
> index 98dd06a..36d0aa0 100644
> --- a/hw/qdev-properties.c
> +++ b/hw/qdev-properties.c
> @@ -822,7 +822,7 @@ static void set_pci_devfn(Object *obj, Visitor *v, void 
> *opaque,
>  {
>      DeviceState *dev = DEVICE(obj);
>      Property *prop = opaque;
> -    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
>      unsigned int slot, fn, n;
>      Error *local_err = NULL;
>      char *str = (char *)"";
> @@ -855,7 +855,7 @@ invalid:
>  
>  static int print_pci_devfn(DeviceState *dev, Property *prop, char *dest, 
> size_t len)
>  {
> -    uint32_t *ptr = qdev_get_prop_ptr(dev, prop);
> +    int32_t *ptr = qdev_get_prop_ptr(dev, prop);
>  
>      if (*ptr == -1) {
>          return snprintf(dest, len, "<unset>");
> @@ -870,11 +870,8 @@ PropertyInfo qdev_prop_pci_devfn = {
>      .print = print_pci_devfn,
>      .get   = get_int32,
>      .set   = set_pci_devfn,
> -    /* FIXME: this should be -1...255, but the address is stored
> -     * into an uint32_t rather than int32_t.
> -     */
> -    .min   = 0,
> -    .max   = 0xFFFFFFFFULL,
> +    .min   = -1,
> +    .max   = 255,
>  };
>  
>  /* --- blocksize --- */
> diff --git a/hw/qdev.h b/hw/qdev.h
> index 4e90119..d07da45 100644
> --- a/hw/qdev.h
> +++ b/hw/qdev.h
> @@ -267,7 +267,7 @@ extern PropertyInfo qdev_prop_blocksize;
>  #define DEFINE_PROP_HEX64(_n, _s, _f, _d)                       \
>      DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_hex64, uint64_t)
>  #define DEFINE_PROP_PCI_DEVFN(_n, _s, _f, _d)                   \
> -    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, uint32_t)
> +    DEFINE_PROP_DEFAULT(_n, _s, _f, _d, qdev_prop_pci_devfn, int32_t)
>  
>  #define DEFINE_PROP_PTR(_n, _s, _f)             \
>      DEFINE_PROP(_n, _s, _f, qdev_prop_ptr, void*)


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg