Re: [Qemu-devel] [PATCH v2 1/4] display: ensure qxl log_buf is a nul terminated string

[Eric Blake]

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