| Message ID | 20170428084237.23960-1-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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. [...]
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
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)
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(-)