* [PULL 1/3] hw/vfio/pci: Fix double free of migration_blocker
2019-11-18 18:46 [PULL 0/3] VFIO fixes 2019-11-18 Alex Williamson
@ 2019-11-18 18:46 ` Alex Williamson
2019-11-18 18:46 ` [PULL 2/3] vfio: don't ignore return value of migrate_add_blocker Alex Williamson
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Alex Williamson @ 2019-11-18 18:46 UTC (permalink / raw)
To: qemu-devel
From: Michal Privoznik <mprivozn@redhat.com>
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)
Fixes: f045a0104c8c ("vfio: unplug failover primary device before migration")
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/vfio/pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index e6569a796850..9c165995df32 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;
}
}
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PULL 2/3] vfio: don't ignore return value of migrate_add_blocker
2019-11-18 18:46 [PULL 0/3] VFIO fixes 2019-11-18 Alex Williamson
2019-11-18 18:46 ` [PULL 1/3] hw/vfio/pci: Fix double free of migration_blocker Alex Williamson
@ 2019-11-18 18:46 ` Alex Williamson
2019-11-18 18:47 ` [PULL 3/3] vfio: vfio-pci requires EDID Alex Williamson
2019-11-19 9:17 ` [PULL 0/3] VFIO fixes 2019-11-18 Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Alex Williamson @ 2019-11-18 18:46 UTC (permalink / raw)
To: qemu-devel
From: Jens Freimann <jfreimann@redhat.com>
When an error occurs in migrate_add_blocker() it sets a
negative return value and uses error pointer we pass in.
Instead of just looking at the error pointer check for a negative return
value and avoid a coverity error because the return value is
set but never used. This fixes CID 1407219.
Reported-by: Coverity (CID 1407219)
Fixes: f045a0104c8c ("vfio: unplug failover primary device before migration")
Signed-off-by: Jens Freimann <jfreimann@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/vfio/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 9c165995df32..0c55883bba77 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2737,7 +2737,7 @@ static void vfio_realize(PCIDevice *pdev, Error **errp)
error_setg(&vdev->migration_blocker,
"VFIO device doesn't support migration");
ret = migrate_add_blocker(vdev->migration_blocker, &err);
- if (err) {
+ if (ret) {
error_propagate(errp, err);
error_free(vdev->migration_blocker);
vdev->migration_blocker = NULL;
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PULL 3/3] vfio: vfio-pci requires EDID
2019-11-18 18:46 [PULL 0/3] VFIO fixes 2019-11-18 Alex Williamson
2019-11-18 18:46 ` [PULL 1/3] hw/vfio/pci: Fix double free of migration_blocker Alex Williamson
2019-11-18 18:46 ` [PULL 2/3] vfio: don't ignore return value of migrate_add_blocker Alex Williamson
@ 2019-11-18 18:47 ` Alex Williamson
2019-11-19 9:17 ` [PULL 0/3] VFIO fixes 2019-11-18 Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Alex Williamson @ 2019-11-18 18:47 UTC (permalink / raw)
To: qemu-devel
From: Paolo Bonzini <pbonzini@redhat.com>
hw/vfio/display.c needs the EDID subsystem, select it.
Cc: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
hw/vfio/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/vfio/Kconfig b/hw/vfio/Kconfig
index 34da2a3cfdd9..f0eaa75ce712 100644
--- a/hw/vfio/Kconfig
+++ b/hw/vfio/Kconfig
@@ -6,6 +6,7 @@ config VFIO_PCI
bool
default y
select VFIO
+ select EDID
depends on LINUX && PCI
config VFIO_CCW
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PULL 0/3] VFIO fixes 2019-11-18
2019-11-18 18:46 [PULL 0/3] VFIO fixes 2019-11-18 Alex Williamson
` (2 preceding siblings ...)
2019-11-18 18:47 ` [PULL 3/3] vfio: vfio-pci requires EDID Alex Williamson
@ 2019-11-19 9:17 ` Peter Maydell
3 siblings, 0 replies; 5+ messages in thread
From: Peter Maydell @ 2019-11-19 9:17 UTC (permalink / raw)
To: Alex Williamson; +Cc: QEMU Developers
On Mon, 18 Nov 2019 at 18:48, Alex Williamson
<alex.williamson@redhat.com> wrote:
>
> The following changes since commit 1bd0f1c9c149c2fb738f381099cec7ad0ee224a9:
>
> Merge remote-tracking branch
> 'remotes/kraxel/tags/seabios-20191118-pull-request' into staging (2019-11-18
> 14:30:24 +0000)
>
> are available in the Git repository at:
>
> git://github.com/awilliam/qemu-vfio.git tags/vfio-fixes-20191118.0
>
> for you to fetch changes up to 29b95c992a569818294478b4616e44b45c67d34e:
>
> vfio: vfio-pci requires EDID (2019-11-18 10:41:49 -0700)
>
> ----------------------------------------------------------------
> VFIO fixes 2019-11-18
>
> - Fix migration blocker double free (Michal Privoznik)
>
> - Use migration_add_blocker() return value (Jens Freimann)
>
> - Depend on EDID for display support (Paolo Bonzini)
>
> ----------------------------------------------------------------
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 5+ messages in thread