Re: [PATCH 1/2] hw/vfio/pci: Fix double free of migration_blocker

[Cornelia Huck]

On Mon, 11 Nov 2019 11:37:41 +0100
Michal Privoznik <address@hidden> 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 <address@hidden>

Fixes: f045a0104c8c ("vfio: unplug failover primary device before migration")

> ---
>  hw/vfio/pci.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Cornelia Huck <address@hidden>