[PULL 0/3] VFIO fixes 2019-11-18

[PULL 0/3] VFIO fixes 2019-11-18

[Alex Williamson]

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)

----------------------------------------------------------------
Jens Freimann (1):
      vfio: don't ignore return value of migrate_add_blocker

Michal Privoznik (1):
      hw/vfio/pci: Fix double free of migration_blocker

Paolo Bonzini (1):
      vfio: vfio-pci requires EDID

 hw/vfio/Kconfig | 1 +
 hw/vfio/pci.c   | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

[PULL 1/3] hw/vfio/pci: Fix double free of migration_blocker

[Alex Williamson]

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;
     }
 }
 

[PULL 2/3] vfio: don't ignore return value of migrate_add_blocker

[Alex Williamson]

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;

[PULL 3/3] vfio: vfio-pci requires EDID

[Alex Williamson]

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

Re: [PULL 0/3] VFIO fixes 2019-11-18

[Peter Maydell]

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