Blame SOURCES/kvm-ui-fix-VNC-client-throttling-when-audio-capture-is-a.patch

5d360b
From a12ad57f4b108dc98987b2002169ccfbbd76721d Mon Sep 17 00:00:00 2001
5d360b
From: "Daniel P. Berrange" <berrange@redhat.com>
5d360b
Date: Thu, 8 Feb 2018 17:50:36 +0100
5d360b
Subject: [PATCH 22/27] ui: fix VNC client throttling when audio capture is
5d360b
 active
5d360b
MIME-Version: 1.0
5d360b
Content-Type: text/plain; charset=UTF-8
5d360b
Content-Transfer-Encoding: 8bit
5d360b
5d360b
RH-Author: Daniel P. Berrange <berrange@redhat.com>
5d360b
Message-id: <20180208175041.5634-23-berrange@redhat.com>
5d360b
Patchwork-id: 78958
5d360b
O-Subject: [RHEL-7.5 qemu-kvm PATCH v1 22/27] ui: fix VNC client throttling when audio capture is active
5d360b
Bugzilla: 1527405
5d360b
RH-Acked-by: Laszlo Ersek <lersek@redhat.com>
5d360b
RH-Acked-by: Gerd Hoffmann <kraxel@redhat.com>
5d360b
RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>
5d360b
5d360b
From: "Daniel P. Berrange" <berrange@redhat.com>
5d360b
5d360b
The VNC server must throttle data sent to the client to prevent the 'output'
5d360b
buffer size growing without bound, if the client stops reading data off the
5d360b
socket (either maliciously or due to stalled/slow network connection).
5d360b
5d360b
The current throttling is very crude because it simply checks whether the
5d360b
output buffer offset is zero. This check must be disabled if audio capture is
5d360b
enabled, because when streaming audio the output buffer offset will rarely be
5d360b
zero due to queued audio data, and so this would starve framebuffer updates.
5d360b
5d360b
As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM.
5d360b
They can first start something in the guest that triggers lots of framebuffer
5d360b
updates eg play a youtube video. Then enable audio capture, and simply never
5d360b
read data back from the server. This can easily make QEMU's VNC server send
5d360b
buffer consume 100MB of RAM per second, until the OOM killer starts reaping
5d360b
processes (hopefully the rogue QEMU process, but it might pick others...).
5d360b
5d360b
To address this we make the throttling more intelligent, so we can throttle
5d360b
when audio capture is active too. To determine how to throttle incremental
5d360b
updates or audio data, we calculate a size threshold. Normally the threshold is
5d360b
the approximate number of bytes associated with a single complete framebuffer
5d360b
update. ie width * height * bytes per pixel. We'll send incremental updates
5d360b
until we hit this threshold, at which point we'll stop sending updates until
5d360b
data has been written to the wire, causing the output buffer offset to fall
5d360b
back below the threshold.
5d360b
5d360b
If audio capture is enabled, we increase the size of the threshold to also
5d360b
allow for upto 1 seconds worth of audio data samples. ie nchannels * bytes
5d360b
per sample * frequency. This allows the output buffer to have a mixture of
5d360b
incremental framebuffer updates and audio data queued, but once the threshold
5d360b
is exceeded, audio data will be dropped and incremental updates will be
5d360b
throttled.
5d360b
5d360b
This unbounded memory growth affects all VNC server configurations supported by
5d360b
QEMU, with no workaround possible. The mitigating factor is that it can only be
5d360b
triggered by a client that has authenticated with the VNC server, and who is
5d360b
able to trigger a large quantity of framebuffer updates or audio samples from
5d360b
the guest OS. Mostly they'll just succeed in getting the OOM killer to kill
5d360b
their own QEMU process, but its possible other processes can get taken out as
5d360b
collateral damage.
5d360b
5d360b
This is a more general variant of the similar unbounded memory usage flaw in
5d360b
the websockets server, that was previously assigned CVE-2017-15268, and fixed
5d360b
in 2.11 by:
5d360b
5d360b
  commit a7b20a8efa28e5f22c26c06cd06c2f12bc863493
5d360b
  Author: Daniel P. Berrange <berrange@redhat.com>
5d360b
  Date:   Mon Oct 9 14:43:42 2017 +0100
5d360b
5d360b
    io: monitor encoutput buffer size from websocket GSource
5d360b
5d360b
This new general memory usage flaw has been assigned CVE-2017-15124, and is
5d360b
partially fixed by this patch.
5d360b
5d360b
RHEL-7 note: minimal context differences in "struct VncState" due to
5d360b
downstream lacking (a) commit fb6ba0d5256c ("qapi event: convert VNC
5d360b
events", 2014-06-23), part of v2.1.0, and (b) commit 8e9b0d24fb98 ("ui:
5d360b
convert VNC websockets to use crypto APIs", 2015-07-08), part of v2.4.0.
5d360b
5d360b
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
5d360b
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
5d360b
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
5d360b
Message-id: 20171218191228.31018-10-berrange@redhat.com
5d360b
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
5d360b
(cherry picked from commit e2b72cb6e0443d90d7ab037858cb6834b6cca852)
5d360b
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
5d360b
---
5d360b
 ui/vnc.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
5d360b
 ui/vnc.h |  6 ++++++
5d360b
 2 files changed, 70 insertions(+), 8 deletions(-)
5d360b
5d360b
diff --git a/ui/vnc.c b/ui/vnc.c
5d360b
index a7ec8cc..952a051 100644
5d360b
--- a/ui/vnc.c
5d360b
+++ b/ui/vnc.c
5d360b
@@ -47,6 +47,7 @@ static VncDisplay *vnc_display; /* needed for info vnc */
5d360b
 
5d360b
 static int vnc_cursor_define(VncState *vs);
5d360b
 static void vnc_release_modifiers(VncState *vs);
5d360b
+static void vnc_update_throttle_offset(VncState *vs);
5d360b
 
5d360b
 static void vnc_set_share_mode(VncState *vs, VncShareMode mode)
5d360b
 {
5d360b
@@ -664,6 +665,7 @@ static void vnc_dpy_switch(DisplayChangeListener *dcl,
5d360b
         vnc_set_area_dirty(vs->dirty, vd, 0, 0,
5d360b
                            vnc_width(vd),
5d360b
                            vnc_height(vd));
5d360b
+        vnc_update_throttle_offset(vs);
5d360b
     }
5d360b
 }
5d360b
 
5d360b
@@ -846,16 +848,67 @@ static int find_and_clear_dirty_height(struct VncState *vs,
5d360b
     return h;
5d360b
 }
5d360b
 
5d360b
+/*
5d360b
+ * Figure out how much pending data we should allow in the output
5d360b
+ * buffer before we throttle incremental display updates, and/or
5d360b
+ * drop audio samples.
5d360b
+ *
5d360b
+ * We allow for equiv of 1 full display's worth of FB updates,
5d360b
+ * and 1 second of audio samples. If audio backlog was larger
5d360b
+ * than that the client would already suffering awful audio
5d360b
+ * glitches, so dropping samples is no worse really).
5d360b
+ */
5d360b
+static void vnc_update_throttle_offset(VncState *vs)
5d360b
+{
5d360b
+    size_t offset =
5d360b
+        vs->client_width * vs->client_height * vs->client_pf.bytes_per_pixel;
5d360b
+
5d360b
+    if (vs->audio_cap) {
5d360b
+        int freq = vs->as.freq;
5d360b
+        /* We don't limit freq when reading settings from client, so
5d360b
+         * it could be upto MAX_INT in size. 48khz is a sensible
5d360b
+         * upper bound for trustworthy clients */
5d360b
+        int bps;
5d360b
+        if (freq > 48000) {
5d360b
+            freq = 48000;
5d360b
+        }
5d360b
+        switch (vs->as.fmt) {
5d360b
+        default:
5d360b
+        case  AUD_FMT_U8:
5d360b
+        case  AUD_FMT_S8:
5d360b
+            bps = 1;
5d360b
+            break;
5d360b
+        case  AUD_FMT_U16:
5d360b
+        case  AUD_FMT_S16:
5d360b
+            bps = 2;
5d360b
+            break;
5d360b
+        case  AUD_FMT_U32:
5d360b
+        case  AUD_FMT_S32:
5d360b
+            bps = 4;
5d360b
+            break;
5d360b
+        }
5d360b
+        offset += freq * bps * vs->as.nchannels;
5d360b
+    }
5d360b
+
5d360b
+    /* Put a floor of 1MB on offset, so that if we have a large pending
5d360b
+     * buffer and the display is resized to a small size & back again
5d360b
+     * we don't suddenly apply a tiny send limit
5d360b
+     */
5d360b
+    offset = MAX(offset, 1024 * 1024);
5d360b
+
5d360b
+    vs->throttle_output_offset = offset;
5d360b
+}
5d360b
+
5d360b
 static bool vnc_should_update(VncState *vs)
5d360b
 {
5d360b
     switch (vs->update) {
5d360b
     case VNC_STATE_UPDATE_NONE:
5d360b
         break;
5d360b
     case VNC_STATE_UPDATE_INCREMENTAL:
5d360b
-        /* Only allow incremental updates if the output buffer
5d360b
-         * is empty, or if audio capture is enabled.
5d360b
+        /* Only allow incremental updates if the pending send queue
5d360b
+         * is less than the permitted threshold
5d360b
          */
5d360b
-        if (!vs->output.offset || vs->audio_cap) {
5d360b
+        if (vs->output.offset < vs->throttle_output_offset) {
5d360b
             return true;
5d360b
         }
5d360b
         break;
5d360b
@@ -963,11 +1016,13 @@ static void audio_capture(void *opaque, void *buf, int size)
5d360b
     VncState *vs = opaque;
5d360b
 
5d360b
     vnc_lock_output(vs);
5d360b
-    vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
5d360b
-    vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
5d360b
-    vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA);
5d360b
-    vnc_write_u32(vs, size);
5d360b
-    vnc_write(vs, buf, size);
5d360b
+    if (vs->output.offset < vs->throttle_output_offset) {
5d360b
+        vnc_write_u8(vs, VNC_MSG_SERVER_QEMU);
5d360b
+        vnc_write_u8(vs, VNC_MSG_SERVER_QEMU_AUDIO);
5d360b
+        vnc_write_u16(vs, VNC_MSG_SERVER_QEMU_AUDIO_DATA);
5d360b
+        vnc_write_u32(vs, size);
5d360b
+        vnc_write(vs, buf, size);
5d360b
+    }
5d360b
     vnc_unlock_output(vs);
5d360b
     vnc_flush(vs);
5d360b
 }
5d360b
@@ -2214,6 +2269,7 @@ static int protocol_client_msg(VncState *vs, uint8_t *data, size_t len)
5d360b
         break;
5d360b
     }
5d360b
 
5d360b
+    vnc_update_throttle_offset(vs);
5d360b
     vnc_read_when(vs, protocol_client_msg, 1);
5d360b
     return 0;
5d360b
 }
5d360b
diff --git a/ui/vnc.h b/ui/vnc.h
5d360b
index f19fd0a..d7eede3 100644
5d360b
--- a/ui/vnc.h
5d360b
+++ b/ui/vnc.h
5d360b
@@ -301,6 +301,12 @@ struct VncState
5d360b
 
5d360b
     QObject *info;
5d360b
 
5d360b
+    /* We allow multiple incremental updates or audio capture
5d360b
+     * samples to be queued in output buffer, provided the
5d360b
+     * buffer size doesn't exceed this threshold. The value
5d360b
+     * is calculating dynamically based on framebuffer size
5d360b
+     * and audio sample settings in vnc_update_throttle_offset() */
5d360b
+    size_t throttle_output_offset;
5d360b
     Buffer output;
5d360b
     Buffer input;
5d360b
 #ifdef CONFIG_VNC_WS
5d360b
-- 
5d360b
1.8.3.1
5d360b