[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/4] display: ensure qxl log_buf is a nul ter
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/4] display: ensure qxl log_buf is a nul terminated string |
Date: |
Fri, 18 Jan 2019 11:40:29 -0600 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 |
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é <address@hidden>
> ---
> hw/display/qxl.c | 3 ++-
> hw/display/trace-events | 2 +-
> 2 files changed, 3 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <address@hidden>
> +++ 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