Re: [Qemu-devel] vnc: heap-use-after-free in vnc_client_io() after vnc_disconnect_finish()

[Philippe Mathieu-Daudé]

On 04/19/2018 09:39 PM, Philippe Mathieu-Daudé wrote:
> On 04/19/2018 01:40 PM, Daniel P. Berrangé wrote:
>> On Thu, Apr 19, 2018 at 06:37:12PM +0200, Marc-André Lureau wrote:
>>> On Tue, Apr 17, 2018 at 7:52 PM, Philippe Mathieu-Daudé <address@hidden> 
>>> wrote:
>>>> running commit ce8d4082054519f2eaac39958edde502860a7fc6:
>>>>
>>>> qemu-system-mips -M malta -m 512 \
>>>>   -kernel vmlinux-3.2.0-4-4kc-malta \
>>>>   -append 'root=/dev/sda1 console=ttyS0' \
>>>>   -hda debian_wheezy_mips_standard.qcow2 \
>>>>   -redir tcp:5556::22 -serial stdio
>>>>
>>>> and connecting with "xtightvncviewer :0" I get when closing vnc:
>>>
>>> This is also true with other targets, ex:
>>> x86_64-softmmu/qemu-system-x86_64 -vnc :0
>>>
>>> Daniel, Gerd, are you looking at it? this looks like a 2.12 regression.
>>
>> I've not had toime to look at it - would be useful if someone can bisect
>> it at least if its a regression since 2.11
> 
> My worst bisect so far, anyway here we go:
> 
> 13e1d0e71e78a925848258391a6e616b6b5ae219 is the first bad commit
> commit 13e1d0e71e78a925848258391a6e616b6b5ae219
> Author: Daniel P. Berrange <address@hidden>
> Date:   Thu Feb 1 16:45:14 2018 +0000
> 
>     ui: convert VNC server to QIONetListener
> 
>     The VNC server already has the ability to listen on multiple sockets.
>     Converting it to use the QIONetListener APIs though, will reduce the
>     amount of code in the VNC server and improve the clarity of what is
>     left.
> 
>     Signed-off-by: Daniel P. Berrange <address@hidden>
>     Message-id: address@hidden
>     Signed-off-by: Gerd Hoffmann <address@hidden>
> 
> 
>>>> ==27686==ERROR: AddressSanitizer: heap-use-after-free on address
>>>> 0x63100003c814 at pc 0x55eed918362a bp 0x7ffdf5c36c80 sp 0x7ffdf5c36c78
>>>> READ of size 4 at 0x63100003c814 thread T0
>>>>     #0 0x55eed9183629 in vnc_client_io /source/qemu/ui/vnc.c:1549
>>>>     #1 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>>> /source/qemu/io/channel-watch.c:84
>>>>     #2 0x7f3e181860f4 in g_main_context_dispatch
>>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>>>     #3 0x55eed95b9799 in glib_pollfds_poll 
>>>> /source/qemu/util/main-loop.c:215
>>>>     #4 0x55eed95b9989 in os_host_main_loop_wait
>>>> /source/qemu/util/main-loop.c:263
>>>>     #5 0x55eed95b9b5f in main_loop_wait /source/qemu/util/main-loop.c:522
>>>>     #6 0x55eed898f7dd in main_loop /source/qemu/vl.c:1943
>>>>     #7 0x55eed89a1f0b in main /source/qemu/vl.c:4734
>>>>     #8 0x7f3dfe094a86 in __libc_start_main
>>>> (/lib/x86_64-linux-gnu/libc.so.6+0x21a86)
>>>>     #9 0x55eed8518db9 in _start
>>>> (/build/qemu/mips-softmmu/qemu-system-mips+0x14f3db9)
>>>>
>>>> 0x63100003c814 is located 20 bytes inside of 75744-byte region
>>>> [0x63100003c800,0x63100004efe0)
>>>> freed by thread T0 here:
>>>>     #0 0x7f3e18b878c8 in __interceptor_free
>>>> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd98c8)
>>>>     #1 0x55eed9181bb0 in vnc_disconnect_finish /source/qemu/ui/vnc.c:1278
>>>>     #2 0x55eed9183225 in vnc_client_read /source/qemu/ui/vnc.c:1511
>>>>     #3 0x55eed91835a9 in vnc_client_io /source/qemu/ui/vnc.c:1541
>>>>     #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>>> /source/qemu/io/channel-watch.c:84
>>>>     #5 0x7f3e181860f4 in g_main_context_dispatch
>>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>>>
>>>> previously allocated by thread T0 here:
>>>>     #0 0x7f3e18b87df8 in calloc
>>>> (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9df8)
>>>>     #1 0x7f3e1818b8b0 in g_malloc0
>>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x518b0)
>>>>     #2 0x55eed9192aa0 in vnc_listen_io /source/qemu/ui/vnc.c:3186
>>>>     #3 0x55eed94b9fd7 in qio_net_listener_channel_func
>>>> /source/qemu/io/net-listener.c:57
>>>>     #4 0x55eed94ae26c in qio_channel_fd_source_dispatch
>>>> /source/qemu/io/channel-watch.c:84
>>>>     #5 0x7f3e181860f4 in g_main_context_dispatch
>>>> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x4c0f4)
>>>>
>>>> SUMMARY: AddressSanitizer: heap-use-after-free
>>>> /source/qemu/ui/vnc.c:1549 in vnc_client_io
>>>> Shadow bytes around the buggy address:
>>>>   0x0c627ffff8b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>>   0x0c627ffff8c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>>   0x0c627ffff8d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>>   0x0c627ffff8e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>>   0x0c627ffff8f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>>>> =>0x0c627ffff900: fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>   0x0c627ffff910: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>   0x0c627ffff920: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>   0x0c627ffff930: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>   0x0c627ffff940: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>   0x0c627ffff950: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
>>>>
>>>> Shadow byte legend (one shadow byte represents 8 application bytes):
>>>>   Heap left redzone:       fa
>>>>   Freed heap region:       fd
>>>> ==26606==ABORTING

Apparently this commit only _triggers_ a latent bug...

What I see is

void vnc_disconnect_finish(VncState *vs)
{
    ...
    g_free(vs);
}

So:

static int vnc_client_read(VncState *vs)
{
    ...
    ret = vnc_client_read_plain(vs);
    if (!ret) {
        if (vs->disconnecting) {
            vnc_disconnect_finish(vs); // free(vs)
            return -1;

Then:

gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
                       GIOCondition condition, void *opaque)
{
    VncState *vs = opaque;
    if (condition & G_IO_IN) {
        if (vnc_client_read(vs) < 0) { // vs freed
            goto end;
        }
    }
    ...
 end:
    if (vs->disconnecting) { // vs use-after-free

Which seems related to:

    commit d49b87f0d1e0520443a990fc610d0f02bc63c556
    Author: Klim Kireev <address@hidden>
    Date:   Wed Feb 7 12:48:44 2018 +0300

        vnc: fix segfault in closed connection handling

        On one of our client's node, due to trying to read from closed ioc,
        a segmentation fault occured. Corresponding backtrace:

        0  object_get_class (address@hidden)
        1  qio_channel_readv_full (ioc=0x0, iov=0x7ffe55277180 ...
        2  qio_channel_read (ioc=<optimized out> ...
        3  vnc_client_read_buf (address@hidden, ...
        4  vnc_client_read_plain (vs=0x55625f3c6000)
        5  vnc_client_read (vs=0x55625f3c6000)
        6  vnc_client_io (ioc=<optimized out>, condition=G_IO_IN, ...
        7  g_main_dispatch (context=0x556251568a50)
        8  g_main_context_dispatch (address@hidden)
        9  glib_pollfds_poll ()
        10 os_host_main_loop_wait (timeout=<optimized out>)
        11 main_loop_wait (address@hidden)
        12 main_loop () at vl.c:1909
        13 main (argc=<optimized out>, argv=<optimized out>, ...

        Having analyzed the coredump, I understood that the reason is that
        ioc_tag is reset on vnc_disconnect_start and ioc is cleaned
        in vnc_disconnect_finish. Between these two events due to some
        reasons the ioc_tag was set again and after vnc_disconnect_finish
        the handler is running with freed ioc,
        which led to the segmentation fault.

        The patch checks vs->disconnecting in places where we call
        qio_channel_add_watch and resets handler if disconnecting == TRUE
        to prevent such an occurrence.