input: limit kbd queue depth
diff mbox

Message ID 20170428084237.23960-1-kraxel@redhat.com
State New
Headers show

Commit Message

Gerd Hoffmann April 28, 2017, 8:42 a.m. UTC
Apply a limit to the number of items we accept into the keyboard queue.

Impact: Without this limit vnc clients can exhaust host memory by
sending keyboard events faster than qemu feeds them to the guest.

Cc: P J P <ppandit@redhat.com>
Cc: Huawei PSIRT <PSIRT@huawei.com>
Reported-by: jiangxin1@huawei.com
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/input.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Daniel P. Berrangé April 28, 2017, 8:49 a.m. UTC | #1
On Fri, Apr 28, 2017 at 10:42:37AM +0200, Gerd Hoffmann wrote:
> Apply a limit to the number of items we accept into the keyboard queue.

Is there a need for similar protection fir mouse input events from VNC ?

> Impact: Without this limit vnc clients can exhaust host memory by
> sending keyboard events faster than qemu feeds them to the guest.

Ability for a remote network client to crash a host by exhausting
memory should be considered a security flaw & have a CVE allocated
for it.

Regards,
Daniel
Gerd Hoffmann April 28, 2017, 9:05 a.m. UTC | #2
On Fr, 2017-04-28 at 09:49 +0100, Daniel P. Berrange wrote:
> On Fri, Apr 28, 2017 at 10:42:37AM +0200, Gerd Hoffmann wrote:
> > Apply a limit to the number of items we accept into the keyboard queue.
> 
> Is there a need for similar protection fir mouse input events from VNC ?

No, there is no delay queue for mouse events.

> > Impact: Without this limit vnc clients can exhaust host memory by
> > sending keyboard events faster than qemu feeds them to the guest.
> 
> Ability for a remote network client to crash a host by exhausting
> memory should be considered a security flaw & have a CVE allocated
> for it.

Sure, it's WIP, Prasit will get one.

cheers,
  Gerd
P J P April 28, 2017, 9:09 a.m. UTC | #3
Hello Dan,

+-- On Fri, 28 Apr 2017, Daniel P. Berrange wrote --+
| Ability for a remote network client to crash a host by exhausting
| memory should be considered a security flaw & have a CVE allocated
| for it.

Yes, I'm getting it assigned.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Markus Armbruster April 28, 2017, 12:27 p.m. UTC | #4
Gerd Hoffmann <kraxel@redhat.com> writes:

> Apply a limit to the number of items we accept into the keyboard queue.
>
> Impact: Without this limit vnc clients can exhaust host memory by
> sending keyboard events faster than qemu feeds them to the guest.
>
> Cc: P J P <ppandit@redhat.com>
> Cc: Huawei PSIRT <PSIRT@huawei.com>
> Reported-by: jiangxin1@huawei.com
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/input.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/ui/input.c b/ui/input.c
> index ed88cda6d6..fb1f404095 100644
> --- a/ui/input.c
> +++ b/ui/input.c
> @@ -41,6 +41,8 @@ static QTAILQ_HEAD(QemuInputEventQueueHead, QemuInputEventQueue) kbd_queue =
>      QTAILQ_HEAD_INITIALIZER(kbd_queue);
>  static QEMUTimer *kbd_timer;
>  static uint32_t kbd_default_delay_ms = 10;
> +static uint32_t queue_count;
> +static uint32_t queue_limit = 1024;

Drive-by comment, feel free to ignore: I'd be tempted to lower the limit
to something comparable to actual hardware, then dumb down the queue to
an array.

[...]
Gerd Hoffmann May 2, 2017, 7:12 a.m. UTC | #5
Hi,

> Drive-by comment, feel free to ignore: I'd be tempted to lower the limit
> to something comparable to actual hardware, then dumb down the queue to
> an array.

Well, the point of this queuing feature is to make automated tests a bit
easier, i.e. a script passes something longish like a install URL to
qemu, and qemu forwards it to the guest with small delays so the
keyboard hardware buffers don't overflow.

So I left he limit relatively high.  Even 1024 isn't as much as it
sounds, each typed char is at least four queue items (keydown, delay,
keyup, delay), in case modifiers are needed even more.  So this gives
buffering room for roughly 150-250 chars typed.

Also lowering the limit to something hardware-comparable is rather
pointless, we have those buffers in the hardware emulation, so we could
just drop the queuing support at input layer altogether.  I don't think
this is a good idea though.

cheers,
  Gerd

Patch
diff mbox

diff --git a/ui/input.c b/ui/input.c
index ed88cda6d6..fb1f404095 100644
--- a/ui/input.c
+++ b/ui/input.c
@@ -41,6 +41,8 @@  static QTAILQ_HEAD(QemuInputEventQueueHead, QemuInputEventQueue) kbd_queue =
     QTAILQ_HEAD_INITIALIZER(kbd_queue);
 static QEMUTimer *kbd_timer;
 static uint32_t kbd_default_delay_ms = 10;
+static uint32_t queue_count;
+static uint32_t queue_limit = 1024;
 
 QemuInputHandlerState *qemu_input_handler_register(DeviceState *dev,
                                                    QemuInputHandler *handler)
@@ -268,6 +270,7 @@  static void qemu_input_queue_process(void *opaque)
             break;
         }
         QTAILQ_REMOVE(queue, item, node);
+        queue_count--;
         g_free(item);
     }
 }
@@ -282,6 +285,7 @@  static void qemu_input_queue_delay(struct QemuInputEventQueueHead *queue,
     item->delay_ms = delay_ms;
     item->timer = timer;
     QTAILQ_INSERT_TAIL(queue, item, node);
+    queue_count++;
 
     if (start_timer) {
         timer_mod(item->timer, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL)
@@ -298,6 +302,7 @@  static void qemu_input_queue_event(struct QemuInputEventQueueHead *queue,
     item->src = src;
     item->evt = evt;
     QTAILQ_INSERT_TAIL(queue, item, node);
+    queue_count++;
 }
 
 static void qemu_input_queue_sync(struct QemuInputEventQueueHead *queue)
@@ -306,6 +311,7 @@  static void qemu_input_queue_sync(struct QemuInputEventQueueHead *queue)
 
     item->type = QEMU_INPUT_QUEUE_SYNC;
     QTAILQ_INSERT_TAIL(queue, item, node);
+    queue_count++;
 }
 
 void qemu_input_event_send_impl(QemuConsole *src, InputEvent *evt)
@@ -381,7 +387,7 @@  void qemu_input_event_send_key(QemuConsole *src, KeyValue *key, bool down)
         qemu_input_event_send(src, evt);
         qemu_input_event_sync();
         qapi_free_InputEvent(evt);
-    } else {
+    } else if (queue_count < queue_limit) {
         qemu_input_queue_event(&kbd_queue, src, evt);
         qemu_input_queue_sync(&kbd_queue);
     }
@@ -409,8 +415,10 @@  void qemu_input_event_send_key_delay(uint32_t delay_ms)
         kbd_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL, qemu_input_queue_process,
                                  &kbd_queue);
     }
-    qemu_input_queue_delay(&kbd_queue, kbd_timer,
-                           delay_ms ? delay_ms : kbd_default_delay_ms);
+    if (queue_count < queue_limit) {
+        qemu_input_queue_delay(&kbd_queue, kbd_timer,
+                               delay_ms ? delay_ms : kbd_default_delay_ms);
+    }
 }
 
 InputEvent *qemu_input_event_new_btn(InputButton btn, bool down)