QEMU vhost-user shutdown suffers from use after free (missing clean shutdown)

All the other threads were shutting down after VM_EXIT

#0 __lll_lock_wait () at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:135
#1 0x00007fe48e71b3f8 in _L_cond_lock_886 () from /lib/x86_64-linux-gnu/libpthread.so.0
#2 0x00007fe48e71b164 in __pthread_mutex_cond_lock (mutex=0x556a92b60800 <qemu_global_mutex>)
    at ../nptl/pthread_mutex_lock.c:79
#3 0x00007fe48e715494 in pthread_cond_wait@@GLIBC_2.3.2 ()
    at ../nptl/sysdeps/unix/sysv/linux/x86_64/pthread_cond_wait.S:259
#4 0x0000556a925bd549 in qemu_cond_wait (cond=<optimized out>,
    mutex=mutex@entry=0x556a92b60800 <qemu_global_mutex>)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/util/qemu-thread-posix.c:132
#5 0x0000556a922898db in qemu_kvm_wait_io_event (cpu=<optimized out>)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/cpus.c:1016

The failing thread has a buggy assert:

(gdb) bt
#0 0x00007fe48e376c37 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1 0x00007fe48e37a028 in __GI_abort () at abort.c:89
#2 0x00007fe48e36fbf6 in __assert_fail_base (
    fmt=0x7fe48e4c4018 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
    assertion=assertion@entry=0x556a926552a8 "ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER", file=file@entry=0x556a926551c8 "/build/qemu-KH8VkI/qemu-2.5+dfsg/net/vhost-user.c", line=line@entry=50,
    function=function@entry=0x556a92655440 <__PRETTY_FUNCTION__.31032> "vhost_user_stop") at assert.c:92
#3 0x00007fe48e36fca2 in __GI___assert_fail (
    assertion=assertion@entry=0x556a926552a8 "ncs[i]->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER", file=file@entry=0x556a926551c8 "/build/qemu-KH8VkI/qemu-2.5+dfsg/net/vhost-user.c", line=line@entry=50,
    function=function@entry=0x556a92655440 <__PRETTY_FUNCTION__.31032> "vhost_user_stop")
    at assert.c:101
#4 0x0000556a924f07e1 in vhost_user_stop (queues=<optimized out>, ncs=<optimized out>)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/net/vhost-user.c:50
#5 0x0000556a924f089a in net_vhost_user_event (opaque=0x556a934b15c0, event=5)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/net/vhost-user.c:192
#6 0x0000556a9237fb4f in tcp_chr_disconnect (chr=0x556a934b0900)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/qemu-char.c:2873
#7 0x0000556a9237fc09 in tcp_chr_sync_read (chr=0x556a934b0900, buf=<optimized out>,
    len=<optimized out>) at /build/qemu-KH8VkI/qemu-2.5+dfsg/qemu-char.c:2920
#8 0x0000556a923814cd in qemu_chr_fe_read_all (s=s@entry=0x556a934b0900,
    buf=buf@entry=0x7ffe4d074bf0 "\v", len=len@entry=12)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/qemu-char.c:239
#9 0x0000556a922ecbf8 in vhost_user_read (msg=msg@entry=0x7ffe4d074bf0, dev=0x556a934dd6d0)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/hw/virtio/vhost-user.c:122
#10 0x0000556a922ed00b in vhost_user_get_vring_base (dev=0x556a934dd6d0, ring=0x7ffe4d074d30)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/hw/virtio/vhost-user.c:366
#11 0x0000556a922e8ed0 in vhost_virtqueue_stop (dev=dev@entry=0x556a934dd6d0,
    vdev=vdev@entry=0x556a93bca458, vq=0x556a934dd808, idx=0)
    at /build/qemu-KH8VkI/qemu-2.5+dfsg/hw/virtio/vhost.c:895
#12 0x0000556a922eb824 in vhost_dev_stop (hdev=hdev@entry=0x556a934dd6d0,
    vdev=vdev@entry=0x556a93bca4...

I'm preparing a PPA for a test package to confirm it fixes the issue and will provide it here soon.

Thanks Rafael,
when you do so I can run additional regression test against it.
You might then also add the debdiff for rewiew if you want to.

As promised, the two PPAs (QEMU for Xenial and for Trusty Mitaka (Cloud Archive)) for tests:

https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637
https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637trustymitaka

I'll provide feedback soon and propose the debdiff. Thanks Christian.

On Xenial there is a security fix out since sept 2017, you'd need to insert that and make you build version ...17~ppa1 instead of 16 as you do now.

Will fix that for the SRU and patch on top of security fix.

This is the debdiff (over latest xenial-security) fixing QEMU on Xenial.

I have also corrected the versioning in PPAs:

https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637/
https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637trustymitaka/

As pointed out by cpaelzer. I had used ~ instead of + on the previous one.

Christian,

Please hold the SRU until I receive good feedback from final user.

Regards,
Rafael

I also can't run the tests this week, so I'll wait anyway.
On review the patch itself seems fine.

It would be nice if for the commit you could add it as link to the upstream repo.
Also your date unlikely is in 2017 :-)

Damn =), 2018 it is. Don't need to say its my first SRU in 2018.
Changed upstream commit id to url and id. Feel free to change during sponsorship.
Tks Christian.

Kicked a test of your updated ppa, takes a few hours and I don't know how soon I get to review the results.

regression test is ok, and I "could" shove it into the SRU queue.
But I'll more likely do that on Monday.
One last set of requests to you Rafael
1. please confirm with your customer that the ppa fix is their solution
2. please check with mdeslaur if you need to group up with a security upload
3. if you have anything better than "stress test the shutdown" on the test description that would be great

Still waiting feedback on this.

Christian, got feedback from final user that they can't reproduce the issue. Wanna go and do the SRU ? They're doing extra tests but the fix looks straightforward to me and they can verify the package in -proposed some more if they want. Agreed ?

Fix is good, fix is upstream, testing seems fine - so yeah I'm good to go on with the SRU in general.
But there is a security update in flight (again) that needs to be released first.
Once it is there you (or I) need to rebase against 1:2.5+dfsg-5ubuntu10.22 and then we can drive this as SRU.

I can do it, could u ping me when the times come ? Just a comment here will
let me know. Tks

On Mon, Feb 19, 2018 at 4:24 AM, ChristianEhrhardt <
<email address hidden>> wrote:

> Fix is good, fix is upstream, testing seems fine - so yeah I'm good to go
> on with the SRU in general.
> But there is a security update in flight (again) that needs to be released
> first.
> Once it is there you (or I) need to rebase against 1:2.5+dfsg-5ubuntu10.22
> and then we can drive this as SRU.
>
>

The former Security update is complete, this is now ready for SRU.
I rebased your change (trivial) onto the latest and added some entries to the dep3 header.

Overall it is good and already pre-tested in multiple ways, also you already have a SRU Template available. I sponsored it to Xenial-proposed for the SRU Team to evaluate.

Ok, shows up in xenial-unapproved.

Thank you very much Christian! I'll wait it to be committed to verify.

Hello Rafael, or anyone else affected,

Accepted qemu into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/qemu/1:2.5+dfsg-5ubuntu10.23 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, without details of your testing we will not be able to proceed.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Unfortunately, a security update regression fix was published that superseded this SRU. A new package will need to be prepared and uploaded.

Re-attaching the same debdiff with a different version, on top of the security fixes that preempted last attempt.

I have received some crash files from the lp test package I provided in the PPA. Waiting for them in order to check if it is the same issue or a different one. Will proceed with SRU after that has been clarified.

By fixing the previous BUG, we continued executing the subsequent logic and found another issue. 'll consider them all part of the same bug since it is all related to vhost-user interface shutdown on QEMU.

And, yes, this is related also to proper memory mapping cleanup (or unmapping) for the vhost-user interface and *could* be related to some other bugs (if core dumps are left behind) like memory leaks and such.

From the stack trace:

Thread #1 13162 (Suspended : Signal : SIGSEGV:Segmentation fault)

__GI___pthread_mutex_lock() at pthread_mutex_lock.c:66 0x7f5cc3f88404
qemu_mutex_lock() at qemu-thread-posix.c:73 0x55606bd5f5a9
qemu_chr_fe_write_all() at qemu-char.c:205 0x55606bb23420 -> CharDriverState *s is NULL and there are no checks
vhost_user_write() at vhost-user.c:195 0x55606ba8e3d3
vhost_user_get_vring_base() at vhost-user.c:364 0x55606ba8f06c
vhost_virtqueue_stop() at vhost.c:895 0x55606ba8af40
vhost_dev_stop() at vhost.c:1,262 0x55606ba8d894
vhost_net_stop_one() at vhost_net.c:293 0x55606ba76248
vhost_net_stop() at vhost_net.c:371 0x55606ba76dfb
virtio_net_vhost_status() at virtio-net.c:150 0x55606ba726e5
virtio_net_set_status() at virtio-net.c:162 0x55606ba726e5
virtio_set_status() at virtio.c:624 0x55606ba873cc
vm_state_notify() at vl.c:1,605 0x55606bb2b1c2
do_vm_stop() at cpus.c:724 0x55606ba2c54a
vm_stop() at cpus.c:1,407 0x55606ba2c54a
main_loop_should_exit() at vl.c:1,883 0x55606b9f8060
main_loop() at vl.c:1,931 0x55606b9f8060
main() at vl.c:4,683 0x55606b9f8060

The seg fault happens because the during the "vhost_user_write" logic to obtain the vring_base address for the virtio shutdown, the character device is gone (likely closed by the other side and/or other thread). With that, qemu_chr_fe_write_all() function tries to write to an empty qemu char device (actually it tries to lock, using an object mutex, but it but it is gone) causing the segfault.

The following upstream commits seem to fix this erratic behaviour...

commit fa394ed625731c18f904578903718bf16617fe92
Author: Marc-André Lureau <email address hidden>
Date: Sat Oct 22 12:52:59 2016 +0300

char: make some qemu_chr_fe skip if no driver

Fixes issue by checking the "CharBackend" (instead of ChardriverState) for NULL.

commit 5345fdb4467816c44f6752b3a1f4e73aa25919f9
Author: Marc-André Lureau <email address hidden>
Date: Sat Oct 22 12:52:55 2016 +0300

char: use qemu_chr_fe* functions with CharBackend argument

Creates the "CharBackend" mechanism for qemu-char.

I'm still verifying if its safe to backport only those commits or any other situation should also be satisfied for proper vhost-user shutdown handling.

Alright, so I removed the proposed patch for Xenial since it was only dealing with half of the issue as you could see by my previous post, about the stack trace on a crash file I got. What I did not say in the last comment was about some other discoveries.

Basically, as I said, the qemu-chr code really implements a CharBackend mechanism (differentiating backend and frontend), and it does add a check for null arguments on some of the qemu-chr functions. The point is that, previously, it was up to the caller to check if argument was good or not (and that is why it doesn't have checkers on 2.5 qemu version).

Nevertheless, I haven't pointed why the argument was null =). On:

vhost_net_stop():

...
    for (i = 0; i < total_queues; i++) {
        vhost_net_stop_one(get_vhost_net(ncs[i].peer), dev);
    }
...

get_vhost_net(ncs[i].peer) returned null because the vhost_net struct was already cleaned up by the qemu char event itself (after the socket was closed). There is an upstream fix for this:

commit e6bcb1b61782cefacecc8b9ee7a5f782373dab2d
Author: Marc-André Lureau <email address hidden>
Date: Wed Jul 27 01:15:12 2016 +0400

    vhost-user: keep vhost_net after a disconnection

And it changes the place where vhost_net struct was freed (to the tap interface cleanup). It also replaces (freeing properly) the vhost_net if the nic is restarted.

I'm attaching the fix for 2 cases, found in 2 stack traces in previous comments, and I'm asking the final user to provide feedback on these fixes. Hopefully the vhost-user shutdown logic will be fixed for good.

Okay, so again, for testing purposes, I have provided the fix in the following PPAs:

https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637/+packages
https://launchpad.net/~inaddy/+archive/ubuntu/lp1743637trustymitaka/+packages

I'm waiting feedback in order to finally ask for the SRU.

Alright Christian,

I got positive feedback from final user. Looks like vhost-user shutdown code doesn't present any other errors in their CI environment. Feel free to include this SRU in the next round of fixes for Xenial.

Cheers
-Rafael

This was regression tested already, it was tested by the STS engineer workign on it and we have postive feedback on a affected user - furthermore the former SRU now cleared on last Friday.
>Xenial releases are not affected (So no need to think on other releases) and the change to Xenial LGTM.

That said sponsoring your change now ...
I see it in X-unapproved, ready for SRU Team review.

Hello Rafael, or anyone else affected,

Accepted qemu into xenial-proposed. The package will build now and be available at https://launchpad.net/ubuntu/+source/qemu/1:2.5+dfsg-5ubuntu10.27 in a few hours, and then in the -proposed repository.

Please help us by testing this new package. See https://wiki.ubuntu.com/Testing/EnableProposed for documentation on how to enable and use -proposed.Your feedback will aid us getting this update out to other Ubuntu users.

If this package fixes the bug for you, please add a comment to this bug, mentioning the version of the package you tested and change the tag from verification-needed-xenial to verification-done-xenial. If it does not fix the bug for you, please add a comment stating that, and change the tag to verification-failed-xenial. In either case, details of your testing will help us make a better decision.

Further information regarding the verification process can be found at https://wiki.ubuntu.com/QATeam/PerformingSRUVerification . Thank you in advance!

Okay,

This was a little trick. I could not make openvswitch + dpdk to work in Xenial.. not sure why :\. I've installed pike openvswitch + dpdk (from Cloud Archive) and tested it with Xenial's qemu (from -proposed, from this case) so I could test this logic (apart from final user confirmation):

https://pastebin.ubuntu.com/p/tnKMSWSQRD/

To make sure I was triggering the vhost-user logic I've used systemtap, that didn't work either for latest Xenial kernel I had (4.4.0-125-generic). So I backported systemtap from artful and make it to run a small stap script for qemu-system-x86_64 binary.

https://pastebin.ubuntu.com/p/cYSD9mc4qM/

$ sudo stap -d /lib/x86_64-linux-gnu/libc-2.23.so -d kernel -g ./kvm-arch.stap

# during qemu startup (when vhost_user_start logic is triggered)

 0x5603689d7230 : net_vhost_user_event+0xc0/0x220 [/usr/bin/qemu-system-x86_64]
 0x5603689d7519 : net_init_vhost_user+0x159/0x270 [/usr/bin/qemu-system-x86_64]
 0x5603689d051e : net_client_init+0x14e/0x340 [/usr/bin/qemu-system-x86_64]
 0x5603689d07ac : net_init_netdev+0x2c/0x70 [/usr/bin/qemu-system-x86_64]
 0x560368a9681a : qemu_opts_foreach+0x6a/0xc0 [/usr/bin/qemu-system-x86_64]
 0x5603689d1197 : net_init_clients+0x67/0xe0 [/usr/bin/qemu-system-x86_64]
 0x560368792f46 : main+0xfe6/0x57a0 [/usr/bin/qemu-system-x86_64]
 0x7f57b3077830 : __libc_start_main+0xf0/0x1d0 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x560368798da9 : _start+0x29/0x30 [/usr/bin/qemu-system-x86_64]

# full shutdown with the tap_cleanup and vhost_user_cleanup logics:

----
 0x559d0c93f4a0 : tap_cleanup+0x0/0xe0 [/usr/bin/qemu-system-x86_64]
 0x559d0c93a1c5 : qemu_del_net_client+0x1a5/0x210 [/usr/bin/qemu-system-x86_64]
 0x559d0c93afed : net_cleanup+0x1d/0x60 [/usr/bin/qemu-system-x86_64]
 0x7f9b84d80ff8 : __run_exit_handlers+0xe8/0x120 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d81045 : exit+0x15/0x20 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d67837 : __libc_start_main+0xf7/0x1d0 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x559d0c702da9 : _start+0x29/0x30 [/usr/bin/qemu-system-x86_64]
----
 0x559d0c940de0 : vhost_user_cleanup+0x0/0x40 [/usr/bin/qemu-system-x86_64]
 0x559d0c93a1c5 : qemu_del_net_client+0x1a5/0x210 [/usr/bin/qemu-system-x86_64]
 0x559d0c93afed : net_cleanup+0x1d/0x60 [/usr/bin/qemu-system-x86_64]
 0x7f9b84d80ff8 : __run_exit_handlers+0xe8/0x120 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d81045 : exit+0x15/0x20 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d67837 : __libc_start_main+0xf7/0x1d0 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x559d0c702da9 : _start+0x29/0x30 [/usr/bin/qemu-system-x86_64]
----
 0x559d0c782780 : vhost_user_cleanup+0x0/0x40 [/usr/bin/qemu-system-x86_64]
 0x559d0c78161c : vhost_dev_cleanup+0x8c/0xc0 [/usr/bin/qemu-system-x86_64]
 0x559d0c940df5 : vhost_user_cleanup+0x15/0x40 [/usr/bin/qemu-system-x86_64]
 0x559d0c93a1c5 : qemu_del_net_client+0x1a5/0x210 [/usr/bin/qemu-system-x86_64]
 0x559d0c93afed : net_cleanup+0x1d/0x60 [/usr/bin/qemu-system-x86_64]
 0x7f9b84d80ff8 : __run_exit_handlers+0xe8/0x120 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d81045 : exit+0x15/0x20 [/lib/x86_64-linux-gnu/libc-2.23.so]
 0x7f9b84d67837 : __libc_start_main+0xf7/0x1d0 [/lib/x86_64-li...

This bug was fixed in the package qemu - 1:2.5+dfsg-5ubuntu10.28

---------------
qemu (1:2.5+dfsg-5ubuntu10.28) xenial-security; urgency=medium

  * SECURITY UPDATE: arbitrary code execution via load_multiboot
    - debian/patches/CVE-2018-7550.patch: handle bss_end_addr being zero in
      hw/i386/multiboot.c.
    - CVE-2018-7550

 -- Marc Deslauriers <email address hidden> Fri, 11 May 2018 13:34:24 -0400