[Qemu-devel] [PATCH 0/2] object_initialize: check size of passed in memory

[Qemu-devel] [PATCH 0/2] object_initialize: check size of passed in memory

[Peter Maydell]

This patchset addresses a concern that came up with Andreas' recent
patches for using embedded objects in some of the ARM CPU devices:
object_initialize() doesn't check that there's actually enough space
for the type being added, so if you have:

struct MyDevice {
   ...
   SomeObject obj;
};

    object_initialize(&mydev->obj, "some-object");

then there's no compile time or runtime check that SomeObject
is really big enough for the "some-object" object -- if the
implementation is changed later then there will be silent
memory corruption.

These patches make object_initialize() a macro which can then
use sizeof(*PTR) to pass the size into the implementation to
be checked.

The virtio patch is worth applying anyway -- it removes some
pointless casts which would otherwise have caused false
positives.

Disclaimer: I've eyeballed all the uses of object_initialize()
but I haven't necessarily tested them all.

Peter Maydell (2):
  virtio: Remove unnecessary OBJECT casts
  qom: Make object_initialize and object_initialize_with_type check
    size

 hw/core/qdev.c             |    2 +-
 hw/s390x/s390-virtio-bus.c |   12 ++++++------
 hw/s390x/virtio-ccw.c      |   14 +++++++-------
 hw/virtio/virtio-pci.c     |   16 ++++++++--------
 include/qom/object.h       |   36 ++++++++++++++++++++++++++++++++++--
 qom/object.c               |    9 +++++----
 6 files changed, 61 insertions(+), 28 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] virtio: Remove unnecessary OBJECT casts

[Peter Maydell]

There's no need to cast the first argument of object_initialize
to Object, and it would defeat the ability to check that the
pointer being passed is to a type large enough for the object.
Remove these unnecessary casts.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/s390x/s390-virtio-bus.c |   12 ++++++------
 hw/s390x/virtio-ccw.c      |   14 +++++++-------
 hw/virtio/virtio-pci.c     |   16 ++++++++--------
 3 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/hw/s390x/s390-virtio-bus.c b/hw/s390x/s390-virtio-bus.c
index f0aa941..e46b8c8 100644
--- a/hw/s390x/s390-virtio-bus.c
+++ b/hw/s390x/s390-virtio-bus.c
@@ -170,7 +170,7 @@ static int s390_virtio_net_init(VirtIOS390Device *s390_dev)
 static void s390_virtio_net_instance_init(Object *obj)
 {
     VirtIONetS390 *dev = VIRTIO_NET_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_NET);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_NET);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -189,7 +189,7 @@ static int s390_virtio_blk_init(VirtIOS390Device *s390_dev)
 static void s390_virtio_blk_instance_init(Object *obj)
 {
     VirtIOBlkS390 *dev = VIRTIO_BLK_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_BLK);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -230,7 +230,7 @@ static int s390_virtio_serial_init(VirtIOS390Device *s390_dev)
 static void s390_virtio_serial_instance_init(Object *obj)
 {
     VirtIOSerialS390 *dev = VIRTIO_SERIAL_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SERIAL);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SERIAL);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -262,7 +262,7 @@ static int s390_virtio_scsi_init(VirtIOS390Device *s390_dev)
 static void s390_virtio_scsi_instance_init(Object *obj)
 {
     VirtIOSCSIS390 *dev = VIRTIO_SCSI_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SCSI);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -283,7 +283,7 @@ static int s390_vhost_scsi_init(VirtIOS390Device *s390_dev)
 static void s390_vhost_scsi_instance_init(Object *obj)
 {
     VHostSCSIS390 *dev = VHOST_SCSI_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VHOST_SCSI);
+    object_initialize(&dev->vdev, TYPE_VHOST_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 #endif
@@ -309,7 +309,7 @@ static int s390_virtio_rng_init(VirtIOS390Device *s390_dev)
 static void s390_virtio_rng_instance_init(Object *obj)
 {
     VirtIORNGS390 *dev = VIRTIO_RNG_S390(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_RNG);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng, NULL);
diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c
index 8835bd4..e3b207f 100644
--- a/hw/s390x/virtio-ccw.c
+++ b/hw/s390x/virtio-ccw.c
@@ -659,7 +659,7 @@ static int virtio_ccw_net_init(VirtioCcwDevice *ccw_dev)
 static void virtio_ccw_net_instance_init(Object *obj)
 {
     VirtIONetCcw *dev = VIRTIO_NET_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_NET);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_NET);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -679,7 +679,7 @@ static int virtio_ccw_blk_init(VirtioCcwDevice *ccw_dev)
 static void virtio_ccw_blk_instance_init(Object *obj)
 {
     VirtIOBlkCcw *dev = VIRTIO_BLK_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_BLK);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -712,7 +712,7 @@ static int virtio_ccw_serial_init(VirtioCcwDevice *ccw_dev)
 static void virtio_ccw_serial_instance_init(Object *obj)
 {
     VirtioSerialCcw *dev = VIRTIO_SERIAL_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SERIAL);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SERIAL);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -758,7 +758,7 @@ static void balloon_ccw_stats_set_poll_interval(Object *obj, struct Visitor *v,
 static void virtio_ccw_balloon_instance_init(Object *obj)
 {
     VirtIOBalloonCcw *dev = VIRTIO_BALLOON_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_BALLOON);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_BALLOON);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 
     object_property_add(obj, "guest-stats", "guest statistics",
@@ -798,7 +798,7 @@ static int virtio_ccw_scsi_init(VirtioCcwDevice *ccw_dev)
 static void virtio_ccw_scsi_instance_init(Object *obj)
 {
     VirtIOSCSICcw *dev = VIRTIO_SCSI_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SCSI);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -819,7 +819,7 @@ static int vhost_ccw_scsi_init(VirtioCcwDevice *ccw_dev)
 static void vhost_ccw_scsi_instance_init(Object *obj)
 {
     VHostSCSICcw *dev = VHOST_SCSI_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VHOST_SCSI);
+    object_initialize(&dev->vdev, TYPE_VHOST_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 #endif
@@ -1170,7 +1170,7 @@ static const TypeInfo vhost_ccw_scsi = {
 static void virtio_ccw_rng_instance_init(Object *obj)
 {
     VirtIORNGCcw *dev = VIRTIO_RNG_CCW(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_RNG);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng, NULL);
diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index d37037e..9052484 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -918,7 +918,7 @@ static void virtio_9p_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_9p_pci_instance_init(Object *obj)
 {
     V9fsPCIState *dev = VIRTIO_9P_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_9P);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_9P);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1078,7 +1078,7 @@ static void virtio_blk_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_blk_pci_instance_init(Object *obj)
 {
     VirtIOBlkPCI *dev = VIRTIO_BLK_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_BLK);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_BLK);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1148,7 +1148,7 @@ static void virtio_scsi_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_scsi_pci_instance_init(Object *obj)
 {
     VirtIOSCSIPCI *dev = VIRTIO_SCSI_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SCSI);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1205,7 +1205,7 @@ static void vhost_scsi_pci_class_init(ObjectClass *klass, void *data)
 static void vhost_scsi_pci_instance_init(Object *obj)
 {
     VHostSCSIPCI *dev = VHOST_SCSI_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VHOST_SCSI);
+    object_initialize(&dev->vdev, TYPE_VHOST_SCSI);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1286,7 +1286,7 @@ static void virtio_balloon_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_balloon_pci_instance_init(Object *obj)
 {
     VirtIOBalloonPCI *dev = VIRTIO_BALLOON_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_BALLOON);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_BALLOON);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 
     object_property_add(obj, "guest-stats", "guest statistics",
@@ -1372,7 +1372,7 @@ static void virtio_serial_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_serial_pci_instance_init(Object *obj)
 {
     VirtIOSerialPCI *dev = VIRTIO_SERIAL_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_SERIAL);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_SERIAL);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1431,7 +1431,7 @@ static void virtio_net_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_net_pci_instance_init(Object *obj)
 {
     VirtIONetPCI *dev = VIRTIO_NET_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_NET);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_NET);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
 }
 
@@ -1487,7 +1487,7 @@ static void virtio_rng_pci_class_init(ObjectClass *klass, void *data)
 static void virtio_rng_initfn(Object *obj)
 {
     VirtIORngPCI *dev = VIRTIO_RNG_PCI(obj);
-    object_initialize(OBJECT(&dev->vdev), TYPE_VIRTIO_RNG);
+    object_initialize(&dev->vdev, TYPE_VIRTIO_RNG);
     object_property_add_child(obj, "virtio-backend", OBJECT(&dev->vdev), NULL);
     object_property_add_link(obj, "rng", TYPE_RNG_BACKEND,
                              (Object **)&dev->vdev.conf.rng, NULL);
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 1/2] virtio: Remove unnecessary OBJECT casts

[Andreas Färber]

Am 23.08.2013 15:38, schrieb Peter Maydell:
> There's no need to cast the first argument of object_initialize
> to Object, and it would defeat the ability to check that the
> pointer being passed is to a type large enough for the object.
> Remove these unnecessary casts.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/s390x/s390-virtio-bus.c |   12 ++++++------
>  hw/s390x/virtio-ccw.c      |   14 +++++++-------
>  hw/virtio/virtio-pci.c     |   16 ++++++++--------
>  3 files changed, 21 insertions(+), 21 deletions(-)

Reviewed-by: Andreas Färber <afaerber@suse.de>

Depending on how we proceed we may want to tweak the commit message.

Andreas

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

[Qemu-devel] [PATCH 2/2] qom: Make object_initialize and object_initialize_with_type check size

[Peter Maydell]

Replace object_initialize and object_initialize_with_type with macro
wrappers which pass the size of the type pointed at by their data
argument, so that we can assert that there is enough memory passed
in to instantiate the object.

We add _unchecked variants of each function for the special cases
where the check is not desired, and change the handful of callsites
that require the _unchecked variant.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/core/qdev.c       |    2 +-
 include/qom/object.h |   36 ++++++++++++++++++++++++++++++++++--
 qom/object.c         |    9 +++++----
 3 files changed, 40 insertions(+), 7 deletions(-)

diff --git a/hw/core/qdev.c b/hw/core/qdev.c
index 758de9f..89e4aa9 100644
--- a/hw/core/qdev.c
+++ b/hw/core/qdev.c
@@ -473,7 +473,7 @@ static void bus_unparent(Object *obj)
 void qbus_create_inplace(void *bus, const char *typename,
                          DeviceState *parent, const char *name)
 {
-    object_initialize(bus, typename);
+    object_initialize_unchecked(bus, typename);
     qbus_realize(bus, parent, name);
 }
 
diff --git a/include/qom/object.h b/include/qom/object.h
index 9b69065..0b451c5 100644
--- a/include/qom/object.h
+++ b/include/qom/object.h
@@ -591,8 +591,24 @@ Object *object_new_with_type(Type type);
  * This function will initialize an object.  The memory for the object should
  * have already been allocated.  The returned object has a reference count of 1,
  * and will be finalized when the last reference is dropped.
+ * The obj pointer should be a pointer to a type whose size is sufficient
+ * for the object; this will be checked.
  */
-void object_initialize_with_type(void *data, Type type);
+#define object_initialize_with_type(PTR, TYPE) \
+    do_object_initialize_with_type(PTR, TYPE, sizeof(*(PTR)))
+
+/**
+ * object_initialize_with_type_unchecked:
+ * @obj: A pointer to the memory to be used for the object.
+ * @type: The type of the object to instantiate.
+ *
+ * Variant of object_initialize_with_type which does not check that the
+ * type which obj is a pointer to has enough space for the object.
+ */
+#define object_initialize_with_type_unchecked(PTR, TYPE) \
+    do_object_initialize_with_type(PTR, TYPE, 0)
+
+void do_object_initialize_with_type(void *data, Type type, size_t datasize);
 
 /**
  * object_initialize:
@@ -602,8 +618,24 @@ void object_initialize_with_type(void *data, Type type);
  * This function will initialize an object.  The memory for the object should
  * have already been allocated.  The returned object has a reference count of 1,
  * and will be finalized when the last reference is dropped.
+ * The obj pointer should be a pointer to a type whose size is sufficient
+ * for the object; this will be checked.
  */
-void object_initialize(void *obj, const char *typename);
+#define object_initialize(PTR, TYPE) \
+    do_object_initialize(PTR, TYPE, sizeof(*(PTR)))
+
+/**
+ * object_initialize_unchecked:
+ * @obj: A pointer to the memory to be used for the object.
+ * @typename: The name of the type of the object to instantiate.
+ *
+ * Variant of object_initialize which does not check that the
+ * type which obj is a pointer to has enough space for the object.
+ */
+#define object_initialize_unchecked(PTR, TYPE) \
+    do_object_initialize(PTR, TYPE, 0)
+
+void do_object_initialize(void *obj, const char *typename, size_t datasize);
 
 /**
  * object_dynamic_cast:
diff --git a/qom/object.c b/qom/object.c
index 74fd241..46f0685 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -311,7 +311,7 @@ static void object_post_init_with_type(Object *obj, TypeImpl *ti)
     }
 }
 
-void object_initialize_with_type(void *data, TypeImpl *type)
+void do_object_initialize_with_type(void *data, TypeImpl *type, size_t datasize)
 {
     Object *obj = data;
 
@@ -320,6 +320,7 @@ void object_initialize_with_type(void *data, TypeImpl *type)
 
     g_assert(type->instance_size >= sizeof(Object));
     g_assert(type->abstract == false);
+    g_assert(datasize == 0 || datasize >= type->instance_size);
 
     memset(obj, 0, type->instance_size);
     obj->class = type->class;
@@ -329,11 +330,11 @@ void object_initialize_with_type(void *data, TypeImpl *type)
     object_post_init_with_type(obj, type);
 }
 
-void object_initialize(void *data, const char *typename)
+void do_object_initialize(void *data, const char *typename, size_t datasize)
 {
     TypeImpl *type = type_get_by_name(typename);
 
-    object_initialize_with_type(data, type);
+    do_object_initialize_with_type(data, type, datasize);
 }
 
 static inline bool object_property_is_child(ObjectProperty *prop)
@@ -424,7 +425,7 @@ Object *object_new_with_type(Type type)
     type_initialize(type);
 
     obj = g_malloc(type->instance_size);
-    object_initialize_with_type(obj, type);
+    object_initialize_with_type_unchecked(obj, type);
     obj->free = g_free;
 
     return obj;
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 0/2] object_initialize: check size of passed in memory

[Andreas Färber]

Am 23.08.2013 15:38, schrieb Peter Maydell:
> This patchset addresses a concern that came up with Andreas' recent
> patches for using embedded objects in some of the ARM CPU devices:
> object_initialize() doesn't check that there's actually enough space
> for the type being added, so if you have:
> 
> struct MyDevice {
>    ...
>    SomeObject obj;
> };
> 
>     object_initialize(&mydev->obj, "some-object");
> 
> then there's no compile time or runtime check that SomeObject
> is really big enough for the "some-object" object -- if the
> implementation is changed later then there will be silent
> memory corruption.
> 
> These patches make object_initialize() a macro which can then
> use sizeof(*PTR) to pass the size into the implementation to
> be checked.

Based on your comment I was already preparing a patch to add an explicit
size argument - there's only 33 users in qemu.git, and it would cover
qbus_create_inplace() and other indirect users as well.

> The virtio patch is worth applying anyway -- it removes some
> pointless casts which would otherwise have caused false
> positives.

Agreed. We shouldn't cast objects before they're initialized. That
OBJECT() is a no-op today I would consider an implementation detail.

Regards,
Andreas

> 
> Disclaimer: I've eyeballed all the uses of object_initialize()
> but I haven't necessarily tested them all.
> 
> Peter Maydell (2):
>   virtio: Remove unnecessary OBJECT casts
>   qom: Make object_initialize and object_initialize_with_type check
>     size
> 
>  hw/core/qdev.c             |    2 +-
>  hw/s390x/s390-virtio-bus.c |   12 ++++++------
>  hw/s390x/virtio-ccw.c      |   14 +++++++-------
>  hw/virtio/virtio-pci.c     |   16 ++++++++--------
>  include/qom/object.h       |   36 ++++++++++++++++++++++++++++++++++--
>  qom/object.c               |    9 +++++----
>  6 files changed, 61 insertions(+), 28 deletions(-)
> 


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