| Message ID | c96b132607f4314ce1efc1af995be300f21ecc04.1573468531.git.mprivozn@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | A pair of memory access problems | expand |
On Mon, 11 Nov 2019 11:37:41 +0100 Michal Privoznik <mprivozn@redhat.com> wrote: > When user tries to hotplug a VFIO device, but the operation fails > somewhere in the middle (in my testing it failed because of > RLIMIT_MEMLOCK forbidding more memory allocation), then a double > free occurs. In vfio_realize() the vdev->migration_blocker is > allocated, then something goes wrong which causes control to jump > onto 'error' label where the error is freed. But the pointer is > left pointing to invalid memory. Later, when > vfio_instance_finalize() is called, the memory is freed again. > > In my testing the second hunk was sufficient to fix the bug, but > I figured the first hunk doesn't hurt either. Better safe than sorry, I guess. > > ==169952== Invalid read of size 8 > ==169952== at 0xA47DCD: error_free (error.c:266) > ==169952== by 0x4E0A18: vfio_instance_finalize (pci.c:3040) > ==169952== by 0x8DF74C: object_deinit (object.c:606) > ==169952== by 0x8DF7BE: object_finalize (object.c:620) > ==169952== by 0x8E0757: object_unref (object.c:1074) > ==169952== by 0x45079C: memory_region_unref (memory.c:1779) > ==169952== by 0x45376B: do_address_space_destroy (memory.c:2793) > ==169952== by 0xA5C600: call_rcu_thread (rcu.c:283) > ==169952== by 0xA427CB: qemu_thread_start (qemu-thread-posix.c:519) > ==169952== by 0x80A8457: start_thread (in /lib64/libpthread-2.29.so) > ==169952== by 0x81C96EE: clone (in /lib64/libc-2.29.so) > ==169952== Address 0x143137e0 is 0 bytes inside a block of size 48 free'd > ==169952== at 0x4A342BB: free (vg_replace_malloc.c:530) > ==169952== by 0xA47E05: error_free (error.c:270) > ==169952== by 0x4E0945: vfio_realize (pci.c:3025) > ==169952== by 0x76A4FF: pci_qdev_realize (pci.c:2099) > ==169952== by 0x689B9A: device_set_realized (qdev.c:876) > ==169952== by 0x8E2C80: property_set_bool (object.c:2080) > ==169952== by 0x8E0EF6: object_property_set (object.c:1272) > ==169952== by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26) > ==169952== by 0x8E11DB: object_property_set_bool (object.c:1338) > ==169952== by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673) > ==169952== by 0x5E81E5: qmp_device_add (qdev-monitor.c:798) > ==169952== by 0x9E18A8: do_qmp_dispatch (qmp-dispatch.c:132) > ==169952== Block was alloc'd at > ==169952== at 0x4A35476: calloc (vg_replace_malloc.c:752) > ==169952== by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6) > ==169952== by 0xA47357: error_setv (error.c:61) > ==169952== by 0xA475D9: error_setg_internal (error.c:97) > ==169952== by 0x4DF8C2: vfio_realize (pci.c:2737) > ==169952== by 0x76A4FF: pci_qdev_realize (pci.c:2099) > ==169952== by 0x689B9A: device_set_realized (qdev.c:876) > ==169952== by 0x8E2C80: property_set_bool (object.c:2080) > ==169952== by 0x8E0EF6: object_property_set (object.c:1272) > ==169952== by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26) > ==169952== by 0x8E11DB: object_property_set_bool (object.c:1338) > ==169952== by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673) > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com> Fixes: f045a0104c8c ("vfio: unplug failover primary device before migration") > --- > hw/vfio/pci.c | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index e6569a7968..9c165995df 100644 --- a/hw/vfio/pci.c +++ b/hw/vfio/pci.c @@ -2740,6 +2740,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp) if (err) { error_propagate(errp, err); error_free(vdev->migration_blocker); + vdev->migration_blocker = NULL; return; } } @@ -3023,6 +3024,7 @@ error: if (vdev->migration_blocker) { migrate_del_blocker(vdev->migration_blocker); error_free(vdev->migration_blocker); + vdev->migration_blocker = NULL; } }
When user tries to hotplug a VFIO device, but the operation fails somewhere in the middle (in my testing it failed because of RLIMIT_MEMLOCK forbidding more memory allocation), then a double free occurs. In vfio_realize() the vdev->migration_blocker is allocated, then something goes wrong which causes control to jump onto 'error' label where the error is freed. But the pointer is left pointing to invalid memory. Later, when vfio_instance_finalize() is called, the memory is freed again. In my testing the second hunk was sufficient to fix the bug, but I figured the first hunk doesn't hurt either. ==169952== Invalid read of size 8 ==169952== at 0xA47DCD: error_free (error.c:266) ==169952== by 0x4E0A18: vfio_instance_finalize (pci.c:3040) ==169952== by 0x8DF74C: object_deinit (object.c:606) ==169952== by 0x8DF7BE: object_finalize (object.c:620) ==169952== by 0x8E0757: object_unref (object.c:1074) ==169952== by 0x45079C: memory_region_unref (memory.c:1779) ==169952== by 0x45376B: do_address_space_destroy (memory.c:2793) ==169952== by 0xA5C600: call_rcu_thread (rcu.c:283) ==169952== by 0xA427CB: qemu_thread_start (qemu-thread-posix.c:519) ==169952== by 0x80A8457: start_thread (in /lib64/libpthread-2.29.so) ==169952== by 0x81C96EE: clone (in /lib64/libc-2.29.so) ==169952== Address 0x143137e0 is 0 bytes inside a block of size 48 free'd ==169952== at 0x4A342BB: free (vg_replace_malloc.c:530) ==169952== by 0xA47E05: error_free (error.c:270) ==169952== by 0x4E0945: vfio_realize (pci.c:3025) ==169952== by 0x76A4FF: pci_qdev_realize (pci.c:2099) ==169952== by 0x689B9A: device_set_realized (qdev.c:876) ==169952== by 0x8E2C80: property_set_bool (object.c:2080) ==169952== by 0x8E0EF6: object_property_set (object.c:1272) ==169952== by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26) ==169952== by 0x8E11DB: object_property_set_bool (object.c:1338) ==169952== by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673) ==169952== by 0x5E81E5: qmp_device_add (qdev-monitor.c:798) ==169952== by 0x9E18A8: do_qmp_dispatch (qmp-dispatch.c:132) ==169952== Block was alloc'd at ==169952== at 0x4A35476: calloc (vg_replace_malloc.c:752) ==169952== by 0x51B1158: g_malloc0 (in /usr/lib64/libglib-2.0.so.0.6000.6) ==169952== by 0xA47357: error_setv (error.c:61) ==169952== by 0xA475D9: error_setg_internal (error.c:97) ==169952== by 0x4DF8C2: vfio_realize (pci.c:2737) ==169952== by 0x76A4FF: pci_qdev_realize (pci.c:2099) ==169952== by 0x689B9A: device_set_realized (qdev.c:876) ==169952== by 0x8E2C80: property_set_bool (object.c:2080) ==169952== by 0x8E0EF6: object_property_set (object.c:1272) ==169952== by 0x8E3FC8: object_property_set_qobject (qom-qobject.c:26) ==169952== by 0x8E11DB: object_property_set_bool (object.c:1338) ==169952== by 0x5E7BDD: qdev_device_add (qdev-monitor.c:673) Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- hw/vfio/pci.c | 2 ++ 1 file changed, 2 insertions(+)