5d360b
From 0f463c3d8c457d6a0bf3b91e48fc4e9162061cf6 Mon Sep 17 00:00:00 2001
5d360b
From: "Daniel P. Berrange" <berrange@redhat.com>
5d360b
Date: Thu, 8 Feb 2018 17:50:37 +0100
5d360b
Subject: [PATCH 23/27] ui: fix VNC client throttling when forced update is
5d360b
 requested
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-24-berrange@redhat.com>
5d360b
Patchwork-id: 78949
5d360b
O-Subject: [RHEL-7.5 qemu-kvm PATCH v1 23/27] ui: fix VNC client throttling when forced update is requested
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 is disabled if the client has requested
5d360b
a forced update, because we want to send these as soon as possible.
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 repeatedly send full framebuffer update
5d360b
requests, but never read data back from the server. This can easily make QEMU's
5d360b
VNC server send buffer consume 100MB of RAM per second, until the OOM killer
5d360b
starts reaping processes (hopefully the rogue QEMU process, but it might pick
5d360b
others...).
5d360b
5d360b
To address this we make the throttling more intelligent, so we can throttle
5d360b
full updates. When we get a forced update request, we keep track of exactly how
5d360b
much data we put on the output buffer. We will not process a subsequent forced
5d360b
update request until this data has been fully sent on the wire. We always allow
5d360b
one forced update request to be in flight, regardless of what data is queued
5d360b
for incremental updates or audio data. The slight complication is that we do
5d360b
not initially know how much data an update will send, as this is done in the
5d360b
background by the VNC job thread. So we must track the fact that the job thread
5d360b
has an update pending, and not process any further updates until this job is
5d360b
has been completed & put data on the output buffer.
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: context differences in "struct VncState" and
5d360b
vnc_jobs_consume_buffer() due to downstream lacking (a) commit
5d360b
fb6ba0d5256c ("qapi event: convert VNC events", 2014-06-23), part of
5d360b
v2.1.0, and (b) commit 04d2529da27d ("ui: convert VNC server to use
5d360b
QIOChannelSocket", 2015-12-18), part of v2.6.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-11-berrange@redhat.com
5d360b
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
5d360b
(cherry picked from commit ada8d2e4369ea49677d8672ac81bce73eefd5b54)
5d360b
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
5d360b
---
5d360b
 ui/vnc-auth-sasl.c |  5 +++++
5d360b
 ui/vnc-jobs.c      |  5 +++++
5d360b
 ui/vnc.c           | 28 ++++++++++++++++++++++++----
5d360b
 ui/vnc.h           |  7 +++++++
5d360b
 4 files changed, 41 insertions(+), 4 deletions(-)
5d360b
5d360b
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
5d360b
index 804b8e7..8188081 100644
5d360b
--- a/ui/vnc-auth-sasl.c
5d360b
+++ b/ui/vnc-auth-sasl.c
5d360b
@@ -76,6 +76,11 @@ long vnc_client_write_sasl(VncState *vs)
5d360b
 
5d360b
     vs->sasl.encodedOffset += ret;
5d360b
     if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
5d360b
+        if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
5d360b
+            vs->force_update_offset = 0;
5d360b
+        } else {
5d360b
+            vs->force_update_offset -= vs->sasl.encodedRawLength;
5d360b
+        }
5d360b
         vs->output.offset -= vs->sasl.encodedRawLength;
5d360b
         vs->sasl.encoded = NULL;
5d360b
         vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
5d360b
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
5d360b
index e553bd9..9705899 100644
5d360b
--- a/ui/vnc-jobs.c
5d360b
+++ b/ui/vnc-jobs.c
5d360b
@@ -170,6 +170,11 @@ void vnc_jobs_consume_buffer(VncState *vs)
5d360b
                                 vnc_client_write, vs);
5d360b
         }
5d360b
         buffer_move(&vs->output, &vs->jobs_buffer);
5d360b
+
5d360b
+        if (vs->job_update == VNC_STATE_UPDATE_FORCE) {
5d360b
+            vs->force_update_offset = vs->output.offset;
5d360b
+        }
5d360b
+        vs->job_update = VNC_STATE_UPDATE_NONE;
5d360b
     }
5d360b
     flush = vs->csock != -1 && vs->abort != true;
5d360b
     vnc_unlock_output(vs);
5d360b
diff --git a/ui/vnc.c b/ui/vnc.c
5d360b
index 952a051..96b6caf 100644
5d360b
--- a/ui/vnc.c
5d360b
+++ b/ui/vnc.c
5d360b
@@ -906,14 +906,28 @@ static bool vnc_should_update(VncState *vs)
5d360b
         break;
5d360b
     case VNC_STATE_UPDATE_INCREMENTAL:
5d360b
         /* Only allow incremental updates if the pending send queue
5d360b
-         * is less than the permitted threshold
5d360b
+         * is less than the permitted threshold, and the job worker
5d360b
+         * is completely idle.
5d360b
          */
5d360b
-        if (vs->output.offset < vs->throttle_output_offset) {
5d360b
+        if (vs->output.offset < vs->throttle_output_offset &&
5d360b
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
5d360b
             return true;
5d360b
         }
5d360b
         break;
5d360b
     case VNC_STATE_UPDATE_FORCE:
5d360b
-        return true;
5d360b
+        /* Only allow forced updates if the pending send queue
5d360b
+         * does not contain a previous forced update, and the
5d360b
+         * job worker is completely idle.
5d360b
+         *
5d360b
+         * Note this means we'll queue a forced update, even if
5d360b
+         * the output buffer size is otherwise over the throttle
5d360b
+         * output limit.
5d360b
+         */
5d360b
+        if (vs->force_update_offset == 0 &&
5d360b
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
5d360b
+            return true;
5d360b
+        }
5d360b
+        break;
5d360b
     }
5d360b
     return false;
5d360b
 }
5d360b
@@ -975,8 +989,9 @@ static int vnc_update_client(VncState *vs, int has_dirty)
5d360b
         }
5d360b
     }
5d360b
 
5d360b
-    vnc_job_push(job);
5d360b
+    vs->job_update = vs->update;
5d360b
     vs->update = VNC_STATE_UPDATE_NONE;
5d360b
+    vnc_job_push(job);
5d360b
     vs->has_dirty = 0;
5d360b
     return n;
5d360b
 }
5d360b
@@ -1241,6 +1256,11 @@ static long vnc_client_write_plain(VncState *vs)
5d360b
     if (!ret)
5d360b
         return 0;
5d360b
 
5d360b
+    if (ret >= vs->force_update_offset) {
5d360b
+        vs->force_update_offset = 0;
5d360b
+    } else {
5d360b
+        vs->force_update_offset -= ret;
5d360b
+    }
5d360b
     buffer_advance(&vs->output, ret);
5d360b
 
5d360b
     if (vs->output.offset == 0) {
5d360b
diff --git a/ui/vnc.h b/ui/vnc.h
5d360b
index d7eede3..70316ba 100644
5d360b
--- a/ui/vnc.h
5d360b
+++ b/ui/vnc.h
5d360b
@@ -268,6 +268,7 @@ struct VncState
5d360b
 
5d360b
     VncDisplay *vd;
5d360b
     VncStateUpdate update; /* Most recent pending request from client */
5d360b
+    VncStateUpdate job_update; /* Currently processed by job thread */
5d360b
     int has_dirty;
5d360b
     uint32_t features;
5d360b
     int absolute;
5d360b
@@ -301,6 +302,12 @@ struct VncState
5d360b
 
5d360b
     QObject *info;
5d360b
 
5d360b
+    /* Job thread bottom half has put data for a forced update
5d360b
+     * into the output buffer. This offset points to the end of
5d360b
+     * the update data in the output buffer. This lets us determine
5d360b
+     * when a force update is fully sent to the client, allowing
5d360b
+     * us to process further forced updates. */
5d360b
+    size_t force_update_offset;
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
-- 
5d360b
1.8.3.1
5d360b