[Qemu-devel] Fwd: [PATCH] QEMU patch for PCI handling bug (invalid free)

[Kevin Wolf]

Am 10.12.2018 um 14:38 hat Matthias Weckbecker geschrieben:
> Hi Kevin,
> 
> I'm attaching a patch for qemu. Read below for details.
> 
> There's a bug in qemu in the PCI bridge handling that can be triggered when
> following the steps below:
> 
>   1) Create some VM (e.g. w/ virsh define)
>   2) Ensure there is a PCI bridge attached, e.g. w/ libvirt like this:
> 
>      <controller type='pci' index='1' model='pci-bridge'>
>       <model name='pci-bridge'/>
>       <target chassisNr='1'/>
>       <alias name='pci'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x01' 
> function='0x1'/>
>      </controller>
> 
>   3) Take a snapshot while it's *running*, like this:
> 
>      % virsh start mips64el-vm
>      % virsh snapshot-create-as mips64el-vm mips64el-snap
> 
>   4) Destroy the VM and revert from snapshot:
> 
>      % virsh destroy mips64el-vm
>      % virsh snapshot-revert mips64el-vm mips64el-snap --running
>        error: internal error: process exited while connecting to monitor: 
> corrupted size vs. prev_size
> 
>   5) qemu-system-mips64el crashes
> 
> The attached patch resolves the issue. Can you maybe review+commit, please?

Hi Matthias,

thanks for sending the patch. The next step is that it needs to be
reviewed on the qemu-devel mailing list (CCed) and then the PCI
subsystem maintainers (Michael and Marcel, CCed as well) can commit it.

Maybe some of the explanation above should actually be moved to the
commit message of the patch itself?

Kevin

----- Forwarded message from Matthias Weckbecker <address@hidden> -----

(gdb) bt
#0  __GI_raise (address@hidden) at ../sysdeps/unix/sysv/linux/raise.c:51
#1  0x00007fde12dfc801 in __GI_abort () at abort.c:79
#2  0x00007fde12e45897 in __libc_message (address@hidden, address@hidden 
"%s\n") at ../sysdeps/posix/libc_fatal.c:181
#3  0x00007fde12e4c90a in malloc_printerr (address@hidden "corrupted size vs. 
prev_size") at malloc.c:5350
#4  0x00007fde12e4cb0c in malloc_consolidate (address@hidden <main_arena>) at 
malloc.c:4456
#5  0x00007fde12e5403b in _int_free (have_lock=0, p=<optimized out>, 
av=0x7fde131a7c40 <main_arena>) at malloc.c:4362
#6  __GI___libc_free (mem=0x55f089173c20) at malloc.c:3124
#7  0x000055f086c85062 in phys_section_destroy (mr=0x55f089173c20) at 
./exec.c:1317
#8  phys_sections_free (map=0x55f0890f1560) at ./exec.c:1325
#9  address_space_dispatch_free (d=0x55f0890f1550) at ./exec.c:2777
#10 0x000055f086cc0509 in flatview_destroy (view=0x55f088a5caf0) at 
./memory.c:301
#11 0x000055f087031dc0 in call_rcu_thread (opaque=<optimized out>) at 
./util/rcu.c:272
#12 0x00007fde131b46db in start_thread (arg=0x7fde0aa39700) at 
pthread_create.c:463
#13 0x00007fde12edd88f in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:95

==13641== Thread 2:                                                             
                                                                                
                                      [0/5041]
==13641== Invalid read of size 8
==13641==    at 0x42755B: memory_region_unref (memory.c:1749)
==13641==    by 0x42755B: flatview_destroy (memory.c:307)
==13641==    by 0x798DBF: call_rcu_thread (rcu.c:272)
==13641==    by 0x97BF6DA: start_thread (pthread_create.c:463)
==13641==    by 0x9AF888E: clone (clone.S:95)
==13641==  Address 0x408e4670 is 64 bytes inside a block of size 1,440 free'd
==13641==    at 0x4C30D3B: free (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13641==    by 0x5E988B: get_pci_config_device (pci.c:491)
==13641==    by 0x660069: vmstate_load_state (vmstate.c:140)
==13641==    by 0x660236: vmstate_load_state (vmstate.c:137)
==13641==    by 0x659994: vmstate_load (savevm.c:748)
==13641==    by 0x65A7B1: qemu_loadvm_section_start_full.isra.11 (savevm.c:1918)
==13641==    by 0x65AA67: qemu_loadvm_state_main.isra.13 (savevm.c:2013)
==13641==    by 0x65D7CE: qemu_loadvm_state (savevm.c:2092)
==13641==    by 0x65E40D: load_snapshot (savevm.c:2406)
==13641==    by 0x3E28C2: main (vl.c:4918)
==13641==  Block was alloc'd at
==13641==    at 0x4C2FB0F: malloc (in 
/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==13641==    by 0x8D35A28: g_malloc (in 
/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5600.3)
==13641==    by 0x5ECA8A: pci_bridge_region_init (pci_bridge.c:187)
==13641==    by 0x5ECEFC: pci_bridge_initfn (pci_bridge.c:384)
==13641==    by 0x5E654F: pci_bridge_dev_realize (pci_bridge_dev.c:59)
==13641==    by 0x5EBED0: pci_qdev_realize (pci.c:2034)
==13641==    by 0x5742D8: device_set_realized (qdev.c:914)
==13641==    by 0x6B8F96: property_set_bool (object.c:1906)
==13641==    by 0x6BD11E: object_property_set_qobject (qom-qobject.c:27)
==13641==    by 0x6BAD7F: object_property_set_bool (object.c:1171)
==13641==    by 0x4FA75D: qdev_device_add (qdev-monitor.c:629)
==13641==    by 0x4FCD36: device_init_func (vl.c:2432)
==13641==


>From 8229eb9cb97a1806e264290e2935261bf23c7f34 Mon Sep 17 00:00:00 2001
From: Matthias Weckbecker <address@hidden>
Date: Mon, 10 Dec 2018 14:00:48 +0100
Subject: [PATCH] hw/pci-bridge: Fix invalid free()

When loadvm'ing a *running* snapshot qemu crashes due to an invalid
free. It's fortunately caught early by glibc heap memory corruption
protection and qemu gets killed with SIGABRT.

This commit fixes the issue.

Signed-off-by: Matthias Weckbecker <address@hidden>
---
 hw/pci/pci_bridge.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index ee9dff2d3a..b9143ac88b 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -241,9 +241,9 @@ void pci_bridge_update_mappings(PCIBridge *br)
      * while another accesses an unaffected region. */
     memory_region_transaction_begin();
     pci_bridge_region_del(br, br->windows);
+    pci_bridge_region_cleanup(br, w);
     br->windows = pci_bridge_region_init(br);
     memory_region_transaction_commit();
-    pci_bridge_region_cleanup(br, w);
 }
 
 /* default write_config function for PCI-to-PCI bridge */
-- 
2.11.0

----- End forwarded message -----