9ae3a8
From 0f463c3d8c457d6a0bf3b91e48fc4e9162061cf6 Mon Sep 17 00:00:00 2001
9ae3a8
From: "Daniel P. Berrange" <berrange@redhat.com>
9ae3a8
Date: Thu, 8 Feb 2018 17:50:37 +0100
9ae3a8
Subject: [PATCH 23/27] ui: fix VNC client throttling when forced update is
9ae3a8
 requested
9ae3a8
MIME-Version: 1.0
9ae3a8
Content-Type: text/plain; charset=UTF-8
9ae3a8
Content-Transfer-Encoding: 8bit
9ae3a8
9ae3a8
RH-Author: Daniel P. Berrange <berrange@redhat.com>
9ae3a8
Message-id: <20180208175041.5634-24-berrange@redhat.com>
9ae3a8
Patchwork-id: 78949
9ae3a8
O-Subject: [RHEL-7.5 qemu-kvm PATCH v1 23/27] ui: fix VNC client throttling when forced update is requested
9ae3a8
Bugzilla: 1527405
9ae3a8
RH-Acked-by: Laszlo Ersek <lersek@redhat.com>
9ae3a8
RH-Acked-by: Gerd Hoffmann <kraxel@redhat.com>
9ae3a8
RH-Acked-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
9ae3a8
From: "Daniel P. Berrange" <berrange@redhat.com>
9ae3a8
9ae3a8
The VNC server must throttle data sent to the client to prevent the 'output'
9ae3a8
buffer size growing without bound, if the client stops reading data off the
9ae3a8
socket (either maliciously or due to stalled/slow network connection).
9ae3a8
9ae3a8
The current throttling is very crude because it simply checks whether the
9ae3a8
output buffer offset is zero. This check is disabled if the client has requested
9ae3a8
a forced update, because we want to send these as soon as possible.
9ae3a8
9ae3a8
As a result, the VNC client can cause QEMU to allocate arbitrary amounts of RAM.
9ae3a8
They can first start something in the guest that triggers lots of framebuffer
9ae3a8
updates eg play a youtube video. Then repeatedly send full framebuffer update
9ae3a8
requests, but never read data back from the server. This can easily make QEMU's
9ae3a8
VNC server send buffer consume 100MB of RAM per second, until the OOM killer
9ae3a8
starts reaping processes (hopefully the rogue QEMU process, but it might pick
9ae3a8
others...).
9ae3a8
9ae3a8
To address this we make the throttling more intelligent, so we can throttle
9ae3a8
full updates. When we get a forced update request, we keep track of exactly how
9ae3a8
much data we put on the output buffer. We will not process a subsequent forced
9ae3a8
update request until this data has been fully sent on the wire. We always allow
9ae3a8
one forced update request to be in flight, regardless of what data is queued
9ae3a8
for incremental updates or audio data. The slight complication is that we do
9ae3a8
not initially know how much data an update will send, as this is done in the
9ae3a8
background by the VNC job thread. So we must track the fact that the job thread
9ae3a8
has an update pending, and not process any further updates until this job is
9ae3a8
has been completed & put data on the output buffer.
9ae3a8
9ae3a8
This unbounded memory growth affects all VNC server configurations supported by
9ae3a8
QEMU, with no workaround possible. The mitigating factor is that it can only be
9ae3a8
triggered by a client that has authenticated with the VNC server, and who is
9ae3a8
able to trigger a large quantity of framebuffer updates or audio samples from
9ae3a8
the guest OS. Mostly they'll just succeed in getting the OOM killer to kill
9ae3a8
their own QEMU process, but its possible other processes can get taken out as
9ae3a8
collateral damage.
9ae3a8
9ae3a8
This is a more general variant of the similar unbounded memory usage flaw in
9ae3a8
the websockets server, that was previously assigned CVE-2017-15268, and fixed
9ae3a8
in 2.11 by:
9ae3a8
9ae3a8
  commit a7b20a8efa28e5f22c26c06cd06c2f12bc863493
9ae3a8
  Author: Daniel P. Berrange <berrange@redhat.com>
9ae3a8
  Date:   Mon Oct 9 14:43:42 2017 +0100
9ae3a8
9ae3a8
    io: monitor encoutput buffer size from websocket GSource
9ae3a8
9ae3a8
This new general memory usage flaw has been assigned CVE-2017-15124, and is
9ae3a8
partially fixed by this patch.
9ae3a8
9ae3a8
RHEL-7 note: context differences in "struct VncState" and
9ae3a8
vnc_jobs_consume_buffer() due to downstream lacking (a) commit
9ae3a8
fb6ba0d5256c ("qapi event: convert VNC events", 2014-06-23), part of
9ae3a8
v2.1.0, and (b) commit 04d2529da27d ("ui: convert VNC server to use
9ae3a8
QIOChannelSocket", 2015-12-18), part of v2.6.0.
9ae3a8
9ae3a8
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
9ae3a8
Reviewed-by: Darren Kenny <darren.kenny@oracle.com>
9ae3a8
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
9ae3a8
Message-id: 20171218191228.31018-11-berrange@redhat.com
9ae3a8
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
9ae3a8
(cherry picked from commit ada8d2e4369ea49677d8672ac81bce73eefd5b54)
9ae3a8
Signed-off-by: Miroslav Rezanina <mrezanin@redhat.com>
9ae3a8
---
9ae3a8
 ui/vnc-auth-sasl.c |  5 +++++
9ae3a8
 ui/vnc-jobs.c      |  5 +++++
9ae3a8
 ui/vnc.c           | 28 ++++++++++++++++++++++++----
9ae3a8
 ui/vnc.h           |  7 +++++++
9ae3a8
 4 files changed, 41 insertions(+), 4 deletions(-)
9ae3a8
9ae3a8
diff --git a/ui/vnc-auth-sasl.c b/ui/vnc-auth-sasl.c
9ae3a8
index 804b8e7..8188081 100644
9ae3a8
--- a/ui/vnc-auth-sasl.c
9ae3a8
+++ b/ui/vnc-auth-sasl.c
9ae3a8
@@ -76,6 +76,11 @@ long vnc_client_write_sasl(VncState *vs)
9ae3a8
 
9ae3a8
     vs->sasl.encodedOffset += ret;
9ae3a8
     if (vs->sasl.encodedOffset == vs->sasl.encodedLength) {
9ae3a8
+        if (vs->sasl.encodedRawLength >= vs->force_update_offset) {
9ae3a8
+            vs->force_update_offset = 0;
9ae3a8
+        } else {
9ae3a8
+            vs->force_update_offset -= vs->sasl.encodedRawLength;
9ae3a8
+        }
9ae3a8
         vs->output.offset -= vs->sasl.encodedRawLength;
9ae3a8
         vs->sasl.encoded = NULL;
9ae3a8
         vs->sasl.encodedOffset = vs->sasl.encodedLength = 0;
9ae3a8
diff --git a/ui/vnc-jobs.c b/ui/vnc-jobs.c
9ae3a8
index e553bd9..9705899 100644
9ae3a8
--- a/ui/vnc-jobs.c
9ae3a8
+++ b/ui/vnc-jobs.c
9ae3a8
@@ -170,6 +170,11 @@ void vnc_jobs_consume_buffer(VncState *vs)
9ae3a8
                                 vnc_client_write, vs);
9ae3a8
         }
9ae3a8
         buffer_move(&vs->output, &vs->jobs_buffer);
9ae3a8
+
9ae3a8
+        if (vs->job_update == VNC_STATE_UPDATE_FORCE) {
9ae3a8
+            vs->force_update_offset = vs->output.offset;
9ae3a8
+        }
9ae3a8
+        vs->job_update = VNC_STATE_UPDATE_NONE;
9ae3a8
     }
9ae3a8
     flush = vs->csock != -1 && vs->abort != true;
9ae3a8
     vnc_unlock_output(vs);
9ae3a8
diff --git a/ui/vnc.c b/ui/vnc.c
9ae3a8
index 952a051..96b6caf 100644
9ae3a8
--- a/ui/vnc.c
9ae3a8
+++ b/ui/vnc.c
9ae3a8
@@ -906,14 +906,28 @@ static bool vnc_should_update(VncState *vs)
9ae3a8
         break;
9ae3a8
     case VNC_STATE_UPDATE_INCREMENTAL:
9ae3a8
         /* Only allow incremental updates if the pending send queue
9ae3a8
-         * is less than the permitted threshold
9ae3a8
+         * is less than the permitted threshold, and the job worker
9ae3a8
+         * is completely idle.
9ae3a8
          */
9ae3a8
-        if (vs->output.offset < vs->throttle_output_offset) {
9ae3a8
+        if (vs->output.offset < vs->throttle_output_offset &&
9ae3a8
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
9ae3a8
             return true;
9ae3a8
         }
9ae3a8
         break;
9ae3a8
     case VNC_STATE_UPDATE_FORCE:
9ae3a8
-        return true;
9ae3a8
+        /* Only allow forced updates if the pending send queue
9ae3a8
+         * does not contain a previous forced update, and the
9ae3a8
+         * job worker is completely idle.
9ae3a8
+         *
9ae3a8
+         * Note this means we'll queue a forced update, even if
9ae3a8
+         * the output buffer size is otherwise over the throttle
9ae3a8
+         * output limit.
9ae3a8
+         */
9ae3a8
+        if (vs->force_update_offset == 0 &&
9ae3a8
+            vs->job_update == VNC_STATE_UPDATE_NONE) {
9ae3a8
+            return true;
9ae3a8
+        }
9ae3a8
+        break;
9ae3a8
     }
9ae3a8
     return false;
9ae3a8
 }
9ae3a8
@@ -975,8 +989,9 @@ static int vnc_update_client(VncState *vs, int has_dirty)
9ae3a8
         }
9ae3a8
     }
9ae3a8
 
9ae3a8
-    vnc_job_push(job);
9ae3a8
+    vs->job_update = vs->update;
9ae3a8
     vs->update = VNC_STATE_UPDATE_NONE;
9ae3a8
+    vnc_job_push(job);
9ae3a8
     vs->has_dirty = 0;
9ae3a8
     return n;
9ae3a8
 }
9ae3a8
@@ -1241,6 +1256,11 @@ static long vnc_client_write_plain(VncState *vs)
9ae3a8
     if (!ret)
9ae3a8
         return 0;
9ae3a8
 
9ae3a8
+    if (ret >= vs->force_update_offset) {
9ae3a8
+        vs->force_update_offset = 0;
9ae3a8
+    } else {
9ae3a8
+        vs->force_update_offset -= ret;
9ae3a8
+    }
9ae3a8
     buffer_advance(&vs->output, ret);
9ae3a8
 
9ae3a8
     if (vs->output.offset == 0) {
9ae3a8
diff --git a/ui/vnc.h b/ui/vnc.h
9ae3a8
index d7eede3..70316ba 100644
9ae3a8
--- a/ui/vnc.h
9ae3a8
+++ b/ui/vnc.h
9ae3a8
@@ -268,6 +268,7 @@ struct VncState
9ae3a8
 
9ae3a8
     VncDisplay *vd;
9ae3a8
     VncStateUpdate update; /* Most recent pending request from client */
9ae3a8
+    VncStateUpdate job_update; /* Currently processed by job thread */
9ae3a8
     int has_dirty;
9ae3a8
     uint32_t features;
9ae3a8
     int absolute;
9ae3a8
@@ -301,6 +302,12 @@ struct VncState
9ae3a8
 
9ae3a8
     QObject *info;
9ae3a8
 
9ae3a8
+    /* Job thread bottom half has put data for a forced update
9ae3a8
+     * into the output buffer. This offset points to the end of
9ae3a8
+     * the update data in the output buffer. This lets us determine
9ae3a8
+     * when a force update is fully sent to the client, allowing
9ae3a8
+     * us to process further forced updates. */
9ae3a8
+    size_t force_update_offset;
9ae3a8
     /* We allow multiple incremental updates or audio capture
9ae3a8
      * samples to be queued in output buffer, provided the
9ae3a8
      * buffer size doesn't exceed this threshold. The value
9ae3a8
-- 
9ae3a8
1.8.3.1
9ae3a8