On virtio channel "org.qemu.guest_agent.0", parse the json stream until
the VMDUMP_INFO is received and retrieve the dump details.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/sysemu/dump-info.h | 15 +++++++++++++
 dump.c                     |  3 +++
 hw/char/virtio-console.c   | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)
 create mode 100644 include/sysemu/dump-info.h

diff --git a/include/sysemu/dump-info.h b/include/sysemu/dump-info.h
new file mode 100644
index 0000000000..fb1ddff9af
--- /dev/null
+++ b/include/sysemu/dump-info.h
@@ -0,0 +1,15 @@
+#ifndef DUMP_INFO_H
+#define DUMP_INFO_H
+
+typedef struct DumpInfo {
+    bool received;
+    bool has_phys_base;
+    uint64_t phys_base;
+    bool has_text;
+    uint64_t text;
+    char *vmcoreinfo;
+} DumpInfo;
+
+extern DumpInfo dump_info;
+
+#endif /* DUMP_INFO_H */
diff --git a/dump.c b/dump.c
index f7b80d856b..68b406459e 100644
--- a/dump.c
+++ b/dump.c
@@ -20,6 +20,7 @@
 #include "monitor/monitor.h"
 #include "sysemu/kvm.h"
 #include "sysemu/dump.h"
+#include "sysemu/dump-info.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/memory_mapping.h"
 #include "sysemu/cpus.h"
@@ -38,6 +39,8 @@
 #define ELF_MACHINE_UNAME "Unknown"
 #endif
 
+DumpInfo dump_info = { 0, };
+
 uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
 {
     if (s->dump_info.d_endian == ELFDATA2LSB) {
diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 798d9b69fd..796b7c85aa 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -16,6 +16,9 @@
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
 #include "qapi-event.h"
+#include "qapi/qmp/json-streamer.h"
+#include "qapi/qmp/json-parser.h"
+#include "sysemu/dump-info.h"
 
 #define TYPE_VIRTIO_CONSOLE_SERIAL_PORT "virtserialport"
 #define VIRTIO_CONSOLE(obj) \
@@ -26,6 +29,7 @@ typedef struct VirtConsole {
 
     CharBackend chr;
     guint watch;
+    JSONMessageParser parser;
 } VirtConsole;
 
 /*
@@ -49,6 +53,11 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
     VirtConsole *vcon = VIRTIO_CONSOLE(port);
     ssize_t ret;
 
+    if (vcon->parser.emit &&
+        !dump_info.received) {
+        json_message_parser_feed(&vcon->parser, (const char *)buf, len);
+    }
+
     if (!qemu_chr_fe_get_driver(&vcon->chr)) {
         /* If there's no backend, we can just say we consumed all data. */
         return len;
@@ -108,6 +117,11 @@ static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
     DeviceState *dev = DEVICE(port);
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
+    if (guest_connected && !port->guest_connected) {
+        g_free(dump_info.vmcoreinfo);
+        memset(&dump_info, 0, sizeof(dump_info));
+    }
+
     if (!k->is_console) {
         qemu_chr_fe_set_open(&vcon->chr, guest_connected);
     }
@@ -163,6 +177,37 @@ static void chr_event(void *opaque, int event)
     }
 }
 
+
+static void qga_message(JSONMessageParser *parser, GQueue *tokens)
+{
+    /* VirtConsole *vcon = container_of(parser, VirtConsole, parser); */
+    QObject *obj;
+    QDict *msg, *data;
+    const char *event;
+
+    obj = json_parser_parse(tokens, NULL);
+    msg = qobject_to_qdict(obj);
+    if (!msg) {
+        error_report("JSON parsing failed");
+        return;
+    }
+
+    event = qdict_get_try_str(msg, "event");
+    data = qdict_get_qdict(msg, "data");
+    if (event && g_str_equal(event, "VMDUMP_INFO") && data) {
+        dump_info.received = true;
+        if (qdict_haskey(data, "phys-base")) {
+            dump_info.has_phys_base = true;
+            dump_info.phys_base = qdict_get_try_uint(data, "phys-base", 0);
+        }
+        if (qdict_haskey(data, "text")) {
+            dump_info.has_text = true;
+            dump_info.text = qdict_get_try_uint(data, "text", 0);
+        }
+        dump_info.vmcoreinfo = g_strdup(qdict_get_try_str(data, "vmcoreinfo"));
+    }
+}
+
 static void virtconsole_realize(DeviceState *dev, Error **errp)
 {
     VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
@@ -195,6 +240,10 @@ static void virtconsole_realize(DeviceState *dev, Error **errp)
                                      chr_event, vcon, NULL, false);
         }
     }
+
+    if (port->name && g_str_equal(port->name, "org.qemu.guest_agent.0")) {
+        json_message_parser_init(&vcon->parser, qga_message);
+    }
 }
 
 static void virtconsole_unrealize(DeviceState *dev, Error **errp)
@@ -204,6 +253,10 @@ static void virtconsole_unrealize(DeviceState *dev, Error **errp)
     if (vcon->watch) {
         g_source_remove(vcon->watch);
     }
+
+    if (vcon->parser.emit) {
+        json_message_parser_destroy(&vcon->parser);
+    }
 }
 
 static void virtconsole_class_init(ObjectClass *klass, void *data)
-- 
2.12.0.191.gc5d8de91d

On Sat, Mar 11, 2017 at 05:22:52PM +0400, Marc-André Lureau wrote:
> On virtio channel "org.qemu.guest_agent.0", parse the json stream until
> the VMDUMP_INFO is received and retrieve the dump details.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/sysemu/dump-info.h | 15 +++++++++++++
>  dump.c                     |  3 +++
>  hw/char/virtio-console.c   | 53 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 71 insertions(+)
>  create mode 100644 include/sysemu/dump-info.h
> 
> diff --git a/include/sysemu/dump-info.h b/include/sysemu/dump-info.h
> new file mode 100644
> index 0000000000..fb1ddff9af
> --- /dev/null
> +++ b/include/sysemu/dump-info.h
> @@ -0,0 +1,15 @@
> +#ifndef DUMP_INFO_H
> +#define DUMP_INFO_H
> +
> +typedef struct DumpInfo {
> +    bool received;
> +    bool has_phys_base;
> +    uint64_t phys_base;
> +    bool has_text;
> +    uint64_t text;
> +    char *vmcoreinfo;
> +} DumpInfo;
> +
> +extern DumpInfo dump_info;
> +
> +#endif /* DUMP_INFO_H */
> diff --git a/dump.c b/dump.c
> index f7b80d856b..68b406459e 100644
> --- a/dump.c
> +++ b/dump.c
> @@ -20,6 +20,7 @@
>  #include "monitor/monitor.h"
>  #include "sysemu/kvm.h"
>  #include "sysemu/dump.h"
> +#include "sysemu/dump-info.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/memory_mapping.h"
>  #include "sysemu/cpus.h"
> @@ -38,6 +39,8 @@
>  #define ELF_MACHINE_UNAME "Unknown"
>  #endif
>  
> +DumpInfo dump_info = { 0, };
> +
>  uint16_t cpu_to_dump16(DumpState *s, uint16_t val)
>  {
>      if (s->dump_info.d_endian == ELFDATA2LSB) {
> diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
> index 798d9b69fd..796b7c85aa 100644
> --- a/hw/char/virtio-console.c
> +++ b/hw/char/virtio-console.c
> @@ -16,6 +16,9 @@
>  #include "trace.h"
>  #include "hw/virtio/virtio-serial.h"
>  #include "qapi-event.h"
> +#include "qapi/qmp/json-streamer.h"
> +#include "qapi/qmp/json-parser.h"
> +#include "sysemu/dump-info.h"
>  
>  #define TYPE_VIRTIO_CONSOLE_SERIAL_PORT "virtserialport"
>  #define VIRTIO_CONSOLE(obj) \
> @@ -26,6 +29,7 @@ typedef struct VirtConsole {
>  
>      CharBackend chr;
>      guint watch;
> +    JSONMessageParser parser;
>  } VirtConsole;
>  
>  /*
> @@ -49,6 +53,11 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
>      VirtConsole *vcon = VIRTIO_CONSOLE(port);
>      ssize_t ret;
>  
> +    if (vcon->parser.emit &&
> +        !dump_info.received) {
> +        json_message_parser_feed(&vcon->parser, (const char *)buf, len);
> +    }

[snip]

so we just continually feed data into the json parser until we see the
event we care about....

What kind of denial of service protection does our JSON parser have. Now
that QEMU is directly parsing JSON from QEMU guest agent, it is exposed
to malicious attack by the guest agent.

eg what happens if the 'vmcoreinfo' string in the JSON doc received from
the guest ends up being 10GB in size ? Is that going to cause our JSON
parser to allocate QString which is 10GB in size which we'll further
try to strdup just below too...


> @@ -163,6 +177,37 @@ static void chr_event(void *opaque, int event)
>      }
>  }
>  
> +
> +static void qga_message(JSONMessageParser *parser, GQueue *tokens)
> +{
> +    /* VirtConsole *vcon = container_of(parser, VirtConsole, parser); */
> +    QObject *obj;
> +    QDict *msg, *data;
> +    const char *event;
> +
> +    obj = json_parser_parse(tokens, NULL);
> +    msg = qobject_to_qdict(obj);
> +    if (!msg) {
> +        error_report("JSON parsing failed");
> +        return;
> +    }
> +
> +    event = qdict_get_try_str(msg, "event");
> +    data = qdict_get_qdict(msg, "data");
> +    if (event && g_str_equal(event, "VMDUMP_INFO") && data) {
> +        dump_info.received = true;
> +        if (qdict_haskey(data, "phys-base")) {
> +            dump_info.has_phys_base = true;
> +            dump_info.phys_base = qdict_get_try_uint(data, "phys-base", 0);
> +        }
> +        if (qdict_haskey(data, "text")) {
> +            dump_info.has_text = true;
> +            dump_info.text = qdict_get_try_uint(data, "text", 0);
> +        }
> +        dump_info.vmcoreinfo = g_strdup(qdict_get_try_str(data, "vmcoreinfo"));
> +    }
> +}


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|

On 04/05/2017 11:12 AM, Daniel P. Berrange wrote:
> On Sat, Mar 11, 2017 at 05:22:52PM +0400, Marc-André Lureau wrote:
>> On virtio channel "org.qemu.guest_agent.0", parse the json stream until
>> the VMDUMP_INFO is received and retrieve the dump details.
>>

> 
> so we just continually feed data into the json parser until we see the
> event we care about....
> 
> What kind of denial of service protection does our JSON parser have. Now
> that QEMU is directly parsing JSON from QEMU guest agent, it is exposed
> to malicious attack by the guest agent.

Our JSON parser rejects input that exceeds various limits:

json-lexer.c:
#define MAX_TOKEN_SIZE (64ULL << 20)

json-streamer.c:
#define MAX_TOKEN_SIZE (64ULL << 20)
#define MAX_TOKEN_COUNT (2ULL << 20)
#define MAX_NESTING (1ULL << 10)

> 
> eg what happens if the 'vmcoreinfo' string in the JSON doc received from
> the guest ends up being 10GB in size ? Is that going to cause our JSON
> parser to allocate QString which is 10GB in size which we'll further
> try to strdup just below too...

The parser will have rejected the guest data long before the 10GB mark.
But our error recovery from that rejection may not be ideal...


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

On Wed, Apr 05, 2017 at 12:06:56PM -0500, Eric Blake wrote:
> On 04/05/2017 11:12 AM, Daniel P. Berrange wrote:
> > On Sat, Mar 11, 2017 at 05:22:52PM +0400, Marc-André Lureau wrote:
> >> On virtio channel "org.qemu.guest_agent.0", parse the json stream until
> >> the VMDUMP_INFO is received and retrieve the dump details.
> >>
> 
> > 
> > so we just continually feed data into the json parser until we see the
> > event we care about....
> > 
> > What kind of denial of service protection does our JSON parser have. Now
> > that QEMU is directly parsing JSON from QEMU guest agent, it is exposed
> > to malicious attack by the guest agent.
> 
> Our JSON parser rejects input that exceeds various limits:
> 
> json-lexer.c:
> #define MAX_TOKEN_SIZE (64ULL << 20)
> 
> json-streamer.c:
> #define MAX_TOKEN_SIZE (64ULL << 20)
> #define MAX_TOKEN_COUNT (2ULL << 20)
> #define MAX_NESTING (1ULL << 10)
> 
> > 
> > eg what happens if the 'vmcoreinfo' string in the JSON doc received from
> > the guest ends up being 10GB in size ? Is that going to cause our JSON
> > parser to allocate QString which is 10GB in size which we'll further
> > try to strdup just below too...
> 
> The parser will have rejected the guest data long before the 10GB mark.
> But our error recovery from that rejection may not be ideal...

Ok, good, we should be pretty much ok then


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|