| Message ID | 20170311132256.22951-18-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
Hi Le 5 avr. 2017 18:13, "Daniel P. Berrange" <berrange@redhat.com> a écrit : 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... I haven't done research on our parsing robustness, but it's not the most complicated task qemu has. The alternative is to have a fixed size message on a different virtio port. But again, once we have a kernel alternative (pstore?) we should switch. > @@ -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...
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
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)
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