On 1/18/19 11:31 AM, Daniel P. Berrangé wrote: > The QXL_IO_LOG command allows the guest to send log messages to the host > via a buffer in the QXLRam struct. QEMU prints these to the console if > the qxl 'guestdebug' option is set to non-zero. It will also feed them > to the trace subsystem if any backends are built-in. > > In both cases the log_buf data will get treated as being as a nul > terminated string, by the printf '%s' format specifier and / or other > code reading the buffer. > > QEMU does nothing to guarantee that the log_buf really is nul terminated, > so there is potential for out of bounds array access. > > This would affect any QEMU which has the log, syslog or ftrace trace > backends built into QEMU. It can only be triggered if the 'qxl_io_log' > trace event is enabled, however, so they are not vulnerable without > specific administrative action to enable this. > > It would also affect QEMU if the 'guestdebug' parameter is set to a > non-zero value, which again is not the default and requires explicit > admin opt-in. > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > --- > hw/display/qxl.c | 3 ++- > hw/display/trace-events | 2 +- > 2 files changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <ebl...@redhat.com>
> +++ b/hw/display/qxl.c
> @@ -1763,7 +1763,8 @@ async_common:
> qxl_set_mode(d, val, 0);
> break;
> case QXL_IO_LOG:
> - trace_qxl_io_log(d->id, d->ram->log_buf);
> + d->ram->log_buf[sizeof(d->ram->log_buf) - 1] = '\0';
May lose a character from the log, but the improved safety is worth it.
> + trace_qxl_io_log(d->id, (const char *)d->ram->log_buf);
> if (d->guestdebug) {
> fprintf(stderr, "qxl/guest-%d: %" PRId64 ": %s", d->id,
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL), d->ram->log_buf);
> diff --git a/hw/display/trace-events b/hw/display/trace-events
> index 5a48c6cb6a..387c6b8931 100644
> --- a/hw/display/trace-events
> +++ b/hw/display/trace-events
> @@ -72,7 +72,7 @@ qxl_interface_update_area_complete_rest(int qid, uint32_t
> num_updated_rects) "%d
> qxl_interface_update_area_complete_overflow(int qid, int max) "%d max=%d"
> qxl_interface_update_area_complete_schedule_bh(int qid, uint32_t num_dirty)
> "%d #dirty=%d"
> qxl_io_destroy_primary_ignored(int qid, const char *mode) "%d %s"
> -qxl_io_log(int qid, const uint8_t *log_buf) "%d %s"
> +qxl_io_log(int qid, const char *log_buf) "%d %s"
> qxl_io_read_unexpected(int qid) "%d"
> qxl_io_unexpected_vga_mode(int qid, uint64_t addr, uint64_t val, const char
> *desc) "%d 0x%"PRIx64"=%"PRIu64" (%s)"
> qxl_io_write(int qid, const char *mode, uint64_t addr, const char *aname,
> uint64_t val, unsigned size, int async) "%d %s addr=%"PRIu64 " (%s)
> val=%"PRIu64" size=%u async=%d"
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
