[PATCH-for-5.0 00/12] various bugfixes

[PATCH-for-5.0 00/12] various bugfixes

[Philippe Mathieu-Daudé]

Collection of bugfixes for 5.0.

Only vhost-user-gpu/grlib_ahb_apb_pnp patches are reviewed.

As 5.0-rc3 is tomorrow, I thought it could help to gather
them and resend altogether.

Regards,

Phil.

Mansour Ahmadi (1):
  hw/block/pflash: Check return value of blk_pwrite()

Philippe Mathieu-Daudé (11):
  Revert "prevent crash when executing guest-file-read with large count"
  qga: Extract guest_file_handle_find() to commands-common.h
  qga: Extract qmp_guest_file_read() to common commands.c
  qga: Restrict guest-file-read count to 48 MB to avoid crashes
  vhost-user-gpu: Release memory returned by vu_queue_pop() with free()
  hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()
  hw/misc/grlib_ahb_apb_pnp: Avoid crash when writing to AHB PnP
    registers
  hw/misc/grlib_ahb_apb_pnp: Fix AHB PnP 8-bit accesses
  hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
  gdbstub: Do not use memset() on GByteArray
  gdbstub: Introduce gdb_get_freg32() to get float32 registers

 qga/qapi-schema.json                    |  6 +++--
 include/exec/gdbstub.h                  | 22 ++++++++++++++++
 qga/commands-common.h                   | 21 +++++++++++++++
 contrib/vhost-user-gpu/vhost-user-gpu.c |  4 +--
 contrib/vhost-user-gpu/virgl.c          |  2 +-
 hw/block/pflash_cfi01.c                 |  8 +++++-
 hw/block/pflash_cfi02.c                 |  8 +++++-
 hw/display/sm501.c                      |  6 +++++
 hw/misc/grlib_ahb_apb_pnp.c             | 11 ++++++++
 hw/openrisc/pic_cpu.c                   |  5 ++--
 qga/commands-posix.c                    | 29 +++++---------------
 qga/commands-win32.c                    | 35 ++++++-------------------
 qga/commands.c                          | 33 +++++++++++++++++++++++
 target/arm/gdbstub.c                    |  3 +--
 target/sh4/gdbstub.c                    |  6 ++---
 target/xtensa/gdbstub.c                 |  6 ++---
 16 files changed, 136 insertions(+), 69 deletions(-)
 create mode 100644 qga/commands-common.h

-- 
2.21.1

[PATCH-for-5.0 01/12] Revert "prevent crash when executing guest-file-read with large count"

[Philippe Mathieu-Daudé]

As noted by Daniel Berrangé in [*], the fix from commit 807e2b6fce
which replaced malloc() by try_malloc() is not enough, the process
can still run out of memory a few line later:

 346     buf = g_try_malloc0(count + 1);
 347     if (!buf) {
 348         error_setg(errp,
 349                    "failed to allocate sufficient memory "
 350                    "to complete the requested service");
 351         return NULL;
 352     }
 353     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
 354     if (!is_ok) {
 355         error_setg_win32(errp, GetLastError(), "failed to read file");
 356         slog("guest-file-read failed, handle %" PRId64, handle);
 357     } else {
 358         buf[read_count] = 0;
 359         read_data = g_new0(GuestFileRead, 1);
                         ^^^^^^

Instead we are going to put a low hard limit on 'count' in the next
commits. This reverts commit 807e2b6fce022707418bc8f61c069d91c613b3d2.

[*] https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg03471.html

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-win32.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index b49920e201..46cea7d1d9 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -343,13 +343,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
 
     fh = gfh->fh;
-    buf = g_try_malloc0(count + 1);
-    if (!buf) {
-        error_setg(errp,
-                   "failed to allocate sufficient memory "
-                   "to complete the requested service");
-        return NULL;
-    }
+    buf = g_malloc0(count + 1);
     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
     if (!is_ok) {
         error_setg_win32(errp, GetLastError(), "failed to read file");
-- 
2.21.1

[PATCH-for-5.0 02/12] qga: Extract guest_file_handle_find() to commands-common.h

[Philippe Mathieu-Daudé]

As we are going to reuse this method, declare it in common
header.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-common.h | 18 ++++++++++++++++++
 qga/commands-posix.c  |  7 ++++---
 qga/commands-win32.c  |  7 ++++---
 3 files changed, 26 insertions(+), 6 deletions(-)
 create mode 100644 qga/commands-common.h

diff --git a/qga/commands-common.h b/qga/commands-common.h
new file mode 100644
index 0000000000..af90e5481e
--- /dev/null
+++ b/qga/commands-common.h
@@ -0,0 +1,18 @@
+/*
+ * QEMU Guest Agent common/cross-platform common commands
+ *
+ * Copyright (c) 2020 Red Hat, Inc.
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#ifndef QGA_COMMANDS_COMMON_H
+#define QGA_COMMANDS_COMMON_H
+
+#include "qga-qapi-types.h"
+
+typedef struct GuestFileHandle GuestFileHandle;
+
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
+
+#endif
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index cc69b82704..c59c32185c 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -26,6 +26,7 @@
 #include "qemu/sockets.h"
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
+#include "commands-common.h"
 
 #ifdef HAVE_UTMPX
 #include <utmpx.h>
@@ -237,12 +238,12 @@ typedef enum {
     RW_STATE_WRITING,
 } RwState;
 
-typedef struct GuestFileHandle {
+struct GuestFileHandle {
     uint64_t id;
     FILE *fh;
     RwState state;
     QTAILQ_ENTRY(GuestFileHandle) next;
-} GuestFileHandle;
+};
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
@@ -268,7 +269,7 @@ static int64_t guest_file_handle_add(FILE *fh, Error **errp)
     return handle;
 }
 
-static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
 {
     GuestFileHandle *gfh;
 
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index 46cea7d1d9..cfaf6b84b8 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -37,6 +37,7 @@
 #include "qemu/queue.h"
 #include "qemu/host-utils.h"
 #include "qemu/base64.h"
+#include "commands-common.h"
 
 #ifndef SHTDN_REASON_FLAG_PLANNED
 #define SHTDN_REASON_FLAG_PLANNED 0x80000000
@@ -50,11 +51,11 @@
 
 #define INVALID_SET_FILE_POINTER ((DWORD)-1)
 
-typedef struct GuestFileHandle {
+struct GuestFileHandle {
     int64_t id;
     HANDLE fh;
     QTAILQ_ENTRY(GuestFileHandle) next;
-} GuestFileHandle;
+};
 
 static struct {
     QTAILQ_HEAD(, GuestFileHandle) filehandles;
@@ -126,7 +127,7 @@ static int64_t guest_file_handle_add(HANDLE fh, Error **errp)
     return handle;
 }
 
-static GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
+GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp)
 {
     GuestFileHandle *gfh;
     QTAILQ_FOREACH(gfh, &guest_file_state.filehandles, next) {
-- 
2.21.1

[PATCH-for-5.0 03/12] qga: Extract qmp_guest_file_read() to common commands.c

[Philippe Mathieu-Daudé]

Extract the common code shared by both POSIX/Win32 implementations.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/commands-common.h |  3 +++
 qga/commands-posix.c  | 22 +++-------------------
 qga/commands-win32.c  | 20 +++-----------------
 qga/commands.c        | 26 ++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 36 deletions(-)

diff --git a/qga/commands-common.h b/qga/commands-common.h
index af90e5481e..90785ed4bb 100644
--- a/qga/commands-common.h
+++ b/qga/commands-common.h
@@ -15,4 +15,7 @@ typedef struct GuestFileHandle GuestFileHandle;
 
 GuestFileHandle *guest_file_handle_find(int64_t id, Error **errp);
 
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp);
+
 #endif
diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index c59c32185c..a52af0315f 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -461,29 +461,14 @@ void qmp_guest_file_close(int64_t handle, Error **errp)
     g_free(gfh);
 }
 
-struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
-                                          int64_t count, Error **errp)
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp)
 {
-    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
     GuestFileRead *read_data = NULL;
     guchar *buf;
-    FILE *fh;
+    FILE *fh = gfh->fh;
     size_t read_count;
 
-    if (!gfh) {
-        return NULL;
-    }
-
-    if (!has_count) {
-        count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0 || count >= UINT32_MAX) {
-        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
-                   count);
-        return NULL;
-    }
-
-    fh = gfh->fh;
-
     /* explicitly flush when switching from writing to reading */
     if (gfh->state == RW_STATE_WRITING) {
         int ret = fflush(fh);
@@ -498,7 +483,6 @@ struct GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     read_count = fread(buf, 1, count, fh);
     if (ferror(fh)) {
         error_setg_errno(errp, errno, "failed to read file");
-        slog("guest-file-read failed, handle: %" PRId64, handle);
     } else {
         buf[read_count] = 0;
         read_data = g_new0(GuestFileRead, 1);
diff --git a/qga/commands-win32.c b/qga/commands-win32.c
index cfaf6b84b8..9717a8d52d 100644
--- a/qga/commands-win32.c
+++ b/qga/commands-win32.c
@@ -322,33 +322,19 @@ void qmp_guest_shutdown(bool has_mode, const char *mode, Error **errp)
     }
 }
 
-GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
-                                   int64_t count, Error **errp)
+GuestFileRead *guest_file_read_unsafe(GuestFileHandle *gfh,
+                                      int64_t count, Error **errp)
 {
     GuestFileRead *read_data = NULL;
     guchar *buf;
-    HANDLE fh;
+    HANDLE fh = gfh->fh;
     bool is_ok;
     DWORD read_count;
-    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
 
-    if (!gfh) {
-        return NULL;
-    }
-    if (!has_count) {
-        count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0 || count >= UINT32_MAX) {
-        error_setg(errp, "value '%" PRId64
-                   "' is invalid for argument count", count);
-        return NULL;
-    }
-
-    fh = gfh->fh;
     buf = g_malloc0(count + 1);
     is_ok = ReadFile(fh, buf, count, &read_count, NULL);
     if (!is_ok) {
         error_setg_win32(errp, GetLastError(), "failed to read file");
-        slog("guest-file-read failed, handle %" PRId64, handle);
     } else {
         buf[read_count] = 0;
         read_data = g_new0(GuestFileRead, 1);
diff --git a/qga/commands.c b/qga/commands.c
index 4471a9f08d..5611117372 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -18,6 +18,7 @@
 #include "qemu/base64.h"
 #include "qemu/cutils.h"
 #include "qemu/atomic.h"
+#include "commands-common.h"
 
 /* Maximum captured guest-exec out_data/err_data - 16MB */
 #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
@@ -547,3 +548,28 @@ error:
     g_free(info);
     return NULL;
 }
+
+GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
+                                   int64_t count, Error **errp)
+{
+    GuestFileHandle *gfh = guest_file_handle_find(handle, errp);
+    GuestFileRead *read_data;
+
+    if (!gfh) {
+        return NULL;
+    }
+    if (!has_count) {
+        count = QGA_READ_COUNT_DEFAULT;
+    } else if (count < 0 || count >= UINT32_MAX) {
+        error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
+                   count);
+        return NULL;
+    }
+
+    read_data = guest_file_read_unsafe(gfh, count, errp);
+    if (!read_data) {
+        slog("guest-file-write failed, handle: %" PRId64, handle);
+    }
+
+    return read_data;
+}
-- 
2.21.1

[PATCH-for-5.0 04/12] qga: Restrict guest-file-read count to 48 MB to avoid crashes

[Philippe Mathieu-Daudé]

On [*] Daniel Berrangé commented:

  The QEMU guest agent protocol is not sensible way to access huge
  files inside the guest. It requires the inefficient process of
  reading the entire data into memory than duplicating it again in
  base64 format, and then copying it again in the JSON serializer /
  monitor code.

  For arbitrary general purpose file access, especially for large
  files, use a real file transfer program or use a network block
  device, not the QEMU guest agent.

To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his
suggestion to put a low, hard limit on "count" in the guest agent
QAPI schema, and don't allow count to be larger than 48 MB.

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html

Fixes: CVE-2018-12617
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 qga/qapi-schema.json | 6 ++++--
 qga/commands.c       | 9 ++++++++-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index f6fcb59f34..7758d9daf8 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -266,11 +266,13 @@
 ##
 # @guest-file-read:
 #
-# Read from an open file in the guest. Data will be base64-encoded
+# Read from an open file in the guest. Data will be base64-encoded.
+# As this command is just for limited, ad-hoc debugging, such as log
+# file access, the number of bytes to read is limited to 10 MB.
 #
 # @handle: filehandle returned by guest-file-open
 #
-# @count: maximum number of bytes to read (default is 4KB)
+# @count: maximum number of bytes to read (default is 4KB, maximum is 10MB)
 #
 # Returns: @GuestFileRead on success.
 #
diff --git a/qga/commands.c b/qga/commands.c
index 5611117372..efc8b90281 100644
--- a/qga/commands.c
+++ b/qga/commands.c
@@ -11,6 +11,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/units.h"
 #include "guest-agent-core.h"
 #include "qga-qapi-commands.h"
 #include "qapi/error.h"
@@ -24,6 +25,12 @@
 #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
 /* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
 #define GUEST_EXEC_IO_SIZE (4*1024)
+/*
+ * Maximum file size to read - 48MB
+ *
+ * (48MB + Base64 3:4 overhead = JSON parser 64 MB limit)
+ */
+#define GUEST_FILE_READ_COUNT_MAX (48 * MiB)
 
 /* Note: in some situations, like with the fsfreeze, logging may be
  * temporarilly disabled. if it is necessary that a command be able
@@ -560,7 +567,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
     }
     if (!has_count) {
         count = QGA_READ_COUNT_DEFAULT;
-    } else if (count < 0 || count >= UINT32_MAX) {
+    } else if (count < 0 || count > GUEST_FILE_READ_COUNT_MAX) {
         error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
                    count);
         return NULL;
-- 
2.21.1

[PATCH-for-5.0 05/12] vhost-user-gpu: Release memory returned by vu_queue_pop() with free()

[Philippe Mathieu-Daudé]

vu_queue_pop() returns memory that must be freed with free().

Cc: qemu-stable@nongnu.org
Reported-by: Coverity (CID 1421887 ALLOC_FREE_MISMATCH)
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 contrib/vhost-user-gpu/vhost-user-gpu.c | 4 ++--
 contrib/vhost-user-gpu/virgl.c          | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
index b45d2019b4..a019d0a9ac 100644
--- a/contrib/vhost-user-gpu/vhost-user-gpu.c
+++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
@@ -848,7 +848,7 @@ vg_handle_ctrl(VuDev *dev, int qidx)
             QTAILQ_INSERT_TAIL(&vg->fenceq, cmd, next);
             vg->inflight++;
         } else {
-            g_free(cmd);
+            free(cmd);
         }
     }
 }
@@ -939,7 +939,7 @@ vg_handle_cursor(VuDev *dev, int qidx)
         }
         vu_queue_push(dev, vq, elem, 0);
         vu_queue_notify(dev, vq);
-        g_free(elem);
+        free(elem);
     }
 }
 
diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
index 43413e29df..b0bc22c3c1 100644
--- a/contrib/vhost-user-gpu/virgl.c
+++ b/contrib/vhost-user-gpu/virgl.c
@@ -519,7 +519,7 @@ virgl_write_fence(void *opaque, uint32_t fence)
         g_debug("FENCE %" PRIu64, cmd->cmd_hdr.fence_id);
         vg_ctrl_response_nodata(g, cmd, VIRTIO_GPU_RESP_OK_NODATA);
         QTAILQ_REMOVE(&g->fenceq, cmd, next);
-        g_free(cmd);
+        free(cmd);
         g->inflight--;
     }
 }
-- 
2.21.1

[PATCH-for-5.0 06/12] hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Coverity points out (CID 1421934) that we are leaking the
memory returned by qemu_allocate_irqs(). We can avoid this
leak by switching to using qdev_init_gpio_in(); the base
class finalize will free the irqs that this allocates under
the hood.

Patch created mechanically using spatch with this script
inspired from commit d6ef883d9d7:

  @@
  typedef qemu_irq;
  identifier irqs, handler;
  expression opaque, count, i;
  @@
  -   qemu_irq *irqs;
      ...
  -   irqs = qemu_allocate_irqs(handler, opaque, count);
  +   qdev_init_gpio_in(DEVICE(opaque), handler, count);
      <+...
  -   irqs[i]
  +   qdev_get_gpio_in(DEVICE(opaque), i)
      ...+>
  ?-  g_free(irqs);

Reported-by: Coverity (CID 1421934 Resource leak)
Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200412212943.4117-4-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/openrisc/pic_cpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/hw/openrisc/pic_cpu.c b/hw/openrisc/pic_cpu.c
index 36f9350830..4b0c92f842 100644
--- a/hw/openrisc/pic_cpu.c
+++ b/hw/openrisc/pic_cpu.c
@@ -52,10 +52,9 @@ static void openrisc_pic_cpu_handler(void *opaque, int irq, int level)
 void cpu_openrisc_pic_init(OpenRISCCPU *cpu)
 {
     int i;
-    qemu_irq *qi;
-    qi = qemu_allocate_irqs(openrisc_pic_cpu_handler, cpu, NR_IRQS);
+    qdev_init_gpio_in(DEVICE(cpu), openrisc_pic_cpu_handler, NR_IRQS);
 
     for (i = 0; i < NR_IRQS; i++) {
-        cpu->env.irq[i] = qi[i];
+        cpu->env.irq[i] = qdev_get_gpio_in(DEVICE(cpu), i);
     }
 }
-- 
2.21.1

[PATCH-for-5.0 07/12] hw/misc/grlib_ahb_apb_pnp: Avoid crash when writing to AHB PnP registers

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Similarly to commit 158b659451 with the APB PnP registers, guests
can crash QEMU when writting to the AHB PnP registers:

  $ echo 'writeb 0xfffff042 69' | qemu-system-sparc -M leon3_generic -S -bios /etc/magic -qtest stdio
  [I 1571938309.932255] OPENED
  [R +0.063474] writeb 0xfffff042 69
  Segmentation fault (core dumped)

  (gdb) bt
  #0  0x0000000000000000 in  ()
  #1  0x0000562999110df4 in memory_region_write_with_attrs_accessor
      (mr=mr@entry=0x56299aa28ea0, addr=66, value=value@entry=0x7fff6abe13b8, size=size@entry=1, shift=<optimized out>, mask=mask@entry=255, attrs=...) at memory.c:503
  #2  0x000056299911095e in access_with_adjusted_size
      (addr=addr@entry=66, value=value@entry=0x7fff6abe13b8, size=size@entry=1, access_size_min=<optimized out>, access_size_max=<optimized out>, access_fn=access_fn@entry=
      0x562999110d70 <memory_region_write_with_attrs_accessor>, mr=0x56299aa28ea0, attrs=...) at memory.c:539
  #3  0x0000562999114fba in memory_region_dispatch_write (mr=mr@entry=0x56299aa28ea0, addr=66, data=<optimized out>, op=<optimized out>, attrs=attrs@entry=...) at memory.c:1482
  #4  0x00005629990c0860 in flatview_write_continue
      (fv=fv@entry=0x56299aa7d8a0, addr=addr@entry=4294963266, attrs=..., ptr=ptr@entry=0x7fff6abe1540, len=len@entry=1, addr1=<optimized out>, l=<optimized out>, mr=0x56299aa28ea0)
      at include/qemu/host-utils.h:164
  #5  0x00005629990c0a76 in flatview_write (fv=0x56299aa7d8a0, addr=4294963266, attrs=..., buf=0x7fff6abe1540, len=1) at exec.c:3165
  #6  0x00005629990c4c1b in address_space_write (as=<optimized out>, addr=<optimized out>, attrs=..., attrs@entry=..., buf=buf@entry=0x7fff6abe1540, len=len@entry=1) at exec.c:3256
  #7  0x000056299910f807 in qtest_process_command (chr=chr@entry=0x5629995ee920 <qtest_chr>, words=words@entry=0x56299acfcfa0) at qtest.c:437

Instead of crashing, log the access as unimplemented.

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200331105048.27989-3-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/misc/grlib_ahb_apb_pnp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/misc/grlib_ahb_apb_pnp.c b/hw/misc/grlib_ahb_apb_pnp.c
index e230e25363..72a8764776 100644
--- a/hw/misc/grlib_ahb_apb_pnp.c
+++ b/hw/misc/grlib_ahb_apb_pnp.c
@@ -136,8 +136,15 @@ static uint64_t grlib_ahb_pnp_read(void *opaque, hwaddr offset, unsigned size)
     return ahb_pnp->regs[offset >> 2];
 }
 
+static void grlib_ahb_pnp_write(void *opaque, hwaddr addr,
+                                uint64_t val, unsigned size)
+{
+    qemu_log_mask(LOG_UNIMP, "%s not implemented\n", __func__);
+}
+
 static const MemoryRegionOps grlib_ahb_pnp_ops = {
     .read       = grlib_ahb_pnp_read,
+    .write      = grlib_ahb_pnp_write,
     .endianness = DEVICE_BIG_ENDIAN,
 };
 
-- 
2.21.1

[PATCH-for-5.0 08/12] hw/misc/grlib_ahb_apb_pnp: Fix AHB PnP 8-bit accesses

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

The Plug & Play region of the AHB/APB bridge can be accessed
by various word size, however the implementation is clearly
restricted to 32-bit:

  static uint64_t grlib_ahb_pnp_read(void *opaque, hwaddr offset, unsigned size)
  {
      AHBPnp *ahb_pnp = GRLIB_AHB_PNP(opaque);

      return ahb_pnp->regs[offset >> 2];
  }

Similarly to commit 0fbe394a64 with the APB PnP registers,
set the MemoryRegionOps::impl min/max fields to 32-bit, so
memory.c::access_with_adjusted_size() can adjust when the
access is not 32-bit.

Reviewed-by: KONRAD Frederic <frederic.konrad@adacore.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200331105048.27989-4-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/misc/grlib_ahb_apb_pnp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/grlib_ahb_apb_pnp.c b/hw/misc/grlib_ahb_apb_pnp.c
index 72a8764776..d22ed00206 100644
--- a/hw/misc/grlib_ahb_apb_pnp.c
+++ b/hw/misc/grlib_ahb_apb_pnp.c
@@ -146,6 +146,10 @@ static const MemoryRegionOps grlib_ahb_pnp_ops = {
     .read       = grlib_ahb_pnp_read,
     .write      = grlib_ahb_pnp_write,
     .endianness = DEVICE_BIG_ENDIAN,
+    .impl = {
+        .min_access_size = 4,
+        .max_access_size = 4,
+    },
 };
 
 static void grlib_ahb_pnp_realize(DeviceState *dev, Error **errp)
-- 
2.21.1

[PATCH-for-5.0 09/12] hw/display/sm501: Avoid heap overflow in sm501_2d_operation()

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Zhang Zi Ming reported a heap overflow in the Drawing Engine of
the SM501 companion chip model, in particular in the COPY_AREA()
macro in sm501_2d_operation().

Add a simple check to avoid the heap overflow.

This fixes:

  =================================================================
  ==20518==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f6f4c3fffff at pc 0x55b1e1d358f0 bp 0x7ffce464dfb0 sp 0x7ffce464dfa8
  READ of size 1 at 0x7f6f4c3fffff thread T0
      #0 0x55b1e1d358ef in sm501_2d_operation hw/display/sm501.c:788:13
      #1 0x55b1e1d32c38 in sm501_2d_engine_write hw/display/sm501.c:1466:13
      #2 0x55b1e0cd19d8 in memory_region_write_accessor memory.c:483:5
      #3 0x55b1e0cd1404 in access_with_adjusted_size memory.c:544:18
      #4 0x55b1e0ccfb9d in memory_region_dispatch_write memory.c:1476:16
      #5 0x55b1e0ae55a8 in flatview_write_continue exec.c:3125:23
      #6 0x55b1e0ad3e87 in flatview_write exec.c:3165:14
      #7 0x55b1e0ad3a24 in address_space_write exec.c:3256:18

  0x7f6f4c3fffff is located 4194303 bytes to the right of 4194304-byte region [0x7f6f4bc00000,0x7f6f4c000000)
  allocated by thread T0 here:
      #0 0x55b1e0a6e715 in __interceptor_posix_memalign (ppc64-softmmu/qemu-system-ppc64+0x19c0715)
      #1 0x55b1e31c1482 in qemu_try_memalign util/oslib-posix.c:189:11
      #2 0x55b1e31c168c in qemu_memalign util/oslib-posix.c:205:27
      #3 0x55b1e11a00b3 in spapr_reallocate_hpt hw/ppc/spapr.c:1560:23
      #4 0x55b1e11a0ce4 in spapr_setup_hpt hw/ppc/spapr.c:1593:5
      #5 0x55b1e11c2fba in spapr_machine_reset hw/ppc/spapr.c:1644:9
      #6 0x55b1e1368b01 in qemu_system_reset softmmu/vl.c:1391:9
      #7 0x55b1e1375af3 in qemu_init softmmu/vl.c:4436:5
      #8 0x55b1e2fc8a59 in main softmmu/main.c:48:5
      #9 0x7f6f8150bf42 in __libc_start_main (/lib64/libc.so.6+0x23f42)

  SUMMARY: AddressSanitizer: heap-buffer-overflow hw/display/sm501.c:788:13 in sm501_2d_operation
  Shadow bytes around the buggy address:
    0x0fee69877fa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0fee69877fb0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0fee69877fc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0fee69877fd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
    0x0fee69877fe0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  =>0x0fee69877ff0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]
    0x0fee69878000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0fee69878010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0fee69878020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0fee69878030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
    0x0fee69878040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  Shadow byte legend (one shadow byte represents 8 application bytes):
    Addressable:           00
    Partially addressable: 01 02 03 04 05 06 07
    Heap left redzone:       fa
    Freed heap region:       fd
    Poisoned by user:        f7
    ASan internal:           fe
  ==20518==ABORTING

Cc: qemu-stable@nongnu.org
Fixes: 07d8a50cb0e ("sm501: add 2D engine copyrect support")
Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1786026
Reported-by: Zhang Zi Ming <1015138407@qq.com>
Acked-by: BALATON Zoltan <balaton@eik.bme.hu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Message-Id: <20200413220100.18628-1-f4bug@amsat.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/display/sm501.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/hw/display/sm501.c b/hw/display/sm501.c
index de0ab9d977..902acb3875 100644
--- a/hw/display/sm501.c
+++ b/hw/display/sm501.c
@@ -726,6 +726,12 @@ static void sm501_2d_operation(SM501State *s)
     int crt = (s->dc_crt_control & SM501_DC_CRT_CONTROL_SEL) ? 1 : 0;
     int fb_len = get_width(s, crt) * get_height(s, crt) * get_bpp(s, crt);
 
+    if (rtl && (src_x < operation_width || src_y < operation_height)) {
+        qemu_log_mask(LOG_GUEST_ERROR, "sm501: Illegal RTL address (%i, %i)\n",
+                      src_x, src_y);
+        return;
+    }
+
     if (addressing != 0x0) {
         printf("%s: only XY addressing is supported.\n", __func__);
         abort();
-- 
2.21.1

[PATCH-for-5.0 10/12] hw/block/pflash: Check return value of blk_pwrite()

[Philippe Mathieu-Daudé]

From: Mansour Ahmadi <mansourweb@gmail.com>

When updating the PFLASH file contents, we should check for a
possible failure of blk_pwrite(). Similar to commit 3a688294e.

Signed-off-by: Mansour Ahmadi <mansourweb@gmail.com>
Message-Id: <20200408003552.58095-1-mansourweb@gmail.com>
[PMD: Add missing "qemu/error-report.h" include and TODO comment]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/block/pflash_cfi01.c | 8 +++++++-
 hw/block/pflash_cfi02.c | 8 +++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
index 24f3bce7ef..be1954c5d8 100644
--- a/hw/block/pflash_cfi01.c
+++ b/hw/block/pflash_cfi01.c
@@ -42,6 +42,7 @@
 #include "hw/qdev-properties.h"
 #include "sysemu/block-backend.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/timer.h"
 #include "qemu/bitops.h"
 #include "qemu/error-report.h"
@@ -399,13 +400,18 @@ static void pflash_update(PFlashCFI01 *pfl, int offset,
                           int size)
 {
     int offset_end;
+    int ret;
     if (pfl->blk) {
         offset_end = offset + size;
         /* widen to sector boundaries */
         offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
         offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
-        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
                    offset_end - offset, 0);
+        if (ret < 0) {
+            /* TODO set error bit in status */
+            error_report("Could not update PFLASH: %s", strerror(-ret));
+        }
     }
 }
 
diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
index 12f18d401a..c6b6f2d082 100644
--- a/hw/block/pflash_cfi02.c
+++ b/hw/block/pflash_cfi02.c
@@ -37,6 +37,7 @@
 #include "hw/block/flash.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "qemu/bitmap.h"
 #include "qemu/timer.h"
 #include "sysemu/block-backend.h"
@@ -393,13 +394,18 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
 static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
 {
     int offset_end;
+    int ret;
     if (pfl->blk) {
         offset_end = offset + size;
         /* widen to sector boundaries */
         offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
         offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
-        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
+        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
                    offset_end - offset, 0);
+        if (ret < 0) {
+            /* TODO set error bit in status */
+            error_report("Could not update PFLASH: %s", strerror(-ret));
+        }
     }
 }
 
-- 
2.21.1

[PATCH-for-5.0 11/12] gdbstub: Do not use memset() on GByteArray

[Philippe Mathieu-Daudé]

Introduce gdb_get_zeroes() to fill a GByteArray with zeroes.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Since v1: Use memset (pm215)
---
 include/exec/gdbstub.h  | 10 ++++++++++
 target/arm/gdbstub.c    |  3 +--
 target/xtensa/gdbstub.c |  6 ++----
 3 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index 30b909ebd2..f44bdd2270 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,16 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_zeroes(GByteArray *array, size_t len)
+{
+    guint oldlen = array->len;
+
+    g_byte_array_set_size(array, oldlen + len);
+    memset(array->data + oldlen, 0, len);
+
+    return len;
+}
+
 /**
  * gdb_get_reg_ptr: get pointer to start of last element
  * @len: length of element
diff --git a/target/arm/gdbstub.c b/target/arm/gdbstub.c
index 8efc535f2a..063551df23 100644
--- a/target/arm/gdbstub.c
+++ b/target/arm/gdbstub.c
@@ -47,8 +47,7 @@ int arm_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         if (gdb_has_xml) {
             return 0;
         }
-        memset(mem_buf, 0, 12);
-        return 12;
+        return gdb_get_zeroes(mem_buf, 12);
     }
     switch (n) {
     case 24:
diff --git a/target/xtensa/gdbstub.c b/target/xtensa/gdbstub.c
index 0ee3feabe5..4d43f1340a 100644
--- a/target/xtensa/gdbstub.c
+++ b/target/xtensa/gdbstub.c
@@ -105,8 +105,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         default:
             qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported size %d\n",
                           __func__, n, reg->size);
-            memset(mem_buf, 0, reg->size);
-            return reg->size;
+            return gdb_get_zeroes(mem_buf, reg->size);
         }
 
     case xtRegisterTypeWindow: /*a*/
@@ -115,8 +114,7 @@ int xtensa_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
     default:
         qemu_log_mask(LOG_UNIMP, "%s from reg %d of unsupported type %d\n",
                       __func__, n, reg->type);
-        memset(mem_buf, 0, reg->size);
-        return reg->size;
+        return gdb_get_zeroes(mem_buf, reg->size);
     }
 }
 
-- 
2.21.1

[PATCH-for-5.0 12/12] gdbstub: Introduce gdb_get_freg32() to get float32 registers

[Philippe Mathieu-Daudé]

Since we now use a GByteArray, we can not use stfl_p() directly.
Introduce the gdb_get_freg32() helper to load a float32 register.

Fixes: a010bdbe719 ("extend GByteArray to read register helpers")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 include/exec/gdbstub.h | 12 ++++++++++++
 target/sh4/gdbstub.c   |  6 ++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/exec/gdbstub.h b/include/exec/gdbstub.h
index f44bdd2270..6d41234071 100644
--- a/include/exec/gdbstub.h
+++ b/include/exec/gdbstub.h
@@ -125,6 +125,18 @@ static inline int gdb_get_reg128(GByteArray *buf, uint64_t val_hi,
     return 16;
 }
 
+static inline int gdb_get_freg32(GByteArray *array, float32 val)
+{
+    uint8_t buf[4];
+
+    QEMU_BUILD_BUG_ON(sizeof(CPU_FloatU) != sizeof(buf));
+
+    stfl_p(buf, val);
+    g_byte_array_append(array, buf, sizeof(buf));
+
+    return sizeof(buf);
+}
+
 static inline int gdb_get_zeroes(GByteArray *array, size_t len)
 {
     guint oldlen = array->len;
diff --git a/target/sh4/gdbstub.c b/target/sh4/gdbstub.c
index 49fc4a0cc6..da95205889 100644
--- a/target/sh4/gdbstub.c
+++ b/target/sh4/gdbstub.c
@@ -58,11 +58,9 @@ int superh_cpu_gdb_read_register(CPUState *cs, GByteArray *mem_buf, int n)
         return gdb_get_regl(mem_buf, env->fpscr);
     case 25 ... 40:
         if (env->fpscr & FPSCR_FR) {
-            stfl_p(mem_buf, env->fregs[n - 9]);
-        } else {
-            stfl_p(mem_buf, env->fregs[n - 25]);
+            return gdb_get_freg32(mem_buf, env->fregs[n - 9]);
         }
-        return 4;
+        return gdb_get_freg32(mem_buf, env->fregs[n - 25]);
     case 41:
         return gdb_get_regl(mem_buf, env->ssr);
     case 42:
-- 
2.21.1

Re: [PATCH-for-5.0 10/12] hw/block/pflash: Check return value of blk_pwrite()

[Mansour Ahmadi]

[-- Attachment #1: Type: text/plain, Size: 3192 bytes --]

Thank you for fixing the patch, Philippe!


On Tue, Apr 14, 2020 at 9:31 AM Philippe Mathieu-Daudé <philmd@redhat.com>
wrote:

> From: Mansour Ahmadi <mansourweb@gmail.com>
>
> When updating the PFLASH file contents, we should check for a
> possible failure of blk_pwrite(). Similar to commit 3a688294e.
>
> Signed-off-by: Mansour Ahmadi <mansourweb@gmail.com>
> Message-Id: <20200408003552.58095-1-mansourweb@gmail.com>
> [PMD: Add missing "qemu/error-report.h" include and TODO comment]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/block/pflash_cfi01.c | 8 +++++++-
>  hw/block/pflash_cfi02.c | 8 +++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 24f3bce7ef..be1954c5d8 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -42,6 +42,7 @@
>  #include "hw/qdev-properties.h"
>  #include "sysemu/block-backend.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/timer.h"
>  #include "qemu/bitops.h"
>  #include "qemu/error-report.h"
> @@ -399,13 +400,18 @@ static void pflash_update(PFlashCFI01 *pfl, int
> offset,
>                            int size)
>  {
>      int offset_end;
> +    int ret;
>      if (pfl->blk) {
>          offset_end = offset + size;
>          /* widen to sector boundaries */
>          offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
>          offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> -        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
> +        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
>                     offset_end - offset, 0);
> +        if (ret < 0) {
> +            /* TODO set error bit in status */
> +            error_report("Could not update PFLASH: %s", strerror(-ret));
> +        }
>      }
>  }
>
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 12f18d401a..c6b6f2d082 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -37,6 +37,7 @@
>  #include "hw/block/flash.h"
>  #include "hw/qdev-properties.h"
>  #include "qapi/error.h"
> +#include "qemu/error-report.h"
>  #include "qemu/bitmap.h"
>  #include "qemu/timer.h"
>  #include "sysemu/block-backend.h"
> @@ -393,13 +394,18 @@ static uint64_t pflash_read(void *opaque, hwaddr
> offset, unsigned int width)
>  static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
>  {
>      int offset_end;
> +    int ret;
>      if (pfl->blk) {
>          offset_end = offset + size;
>          /* widen to sector boundaries */
>          offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
>          offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> -        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
> +        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
>                     offset_end - offset, 0);
> +        if (ret < 0) {
> +            /* TODO set error bit in status */
> +            error_report("Could not update PFLASH: %s", strerror(-ret));
> +        }
>      }
>  }
>
> --
> 2.21.1
>
>

[-- Attachment #2: Type: text/html, Size: 4364 bytes --]

Re: [PATCH-for-5.0 10/12] hw/block/pflash: Check return value of blk_pwrite()

[Philippe Mathieu-Daudé]

On 4/14/20 3:30 PM, Philippe Mathieu-Daudé wrote:
> From: Mansour Ahmadi <mansourweb@gmail.com>
> 
> When updating the PFLASH file contents, we should check for a
> possible failure of blk_pwrite(). Similar to commit 3a688294e.
> 

There is actually a Coverity report for this issue, CID 1357678 
(Unchecked return value) from 2016-07-15 06:28:48:

CID 1357678 (#2 of 2): Unchecked return value (CHECKED_RETURN). 
check_return: Calling blk_pwrite without checking return value (as is 
done elsewhere 52 out of 59 times).

So it seems fair to add:

Reported-by: Coverity (CID 1357678 CHECKED_RETURN)

> Signed-off-by: Mansour Ahmadi <mansourweb@gmail.com>
> Message-Id: <20200408003552.58095-1-mansourweb@gmail.com>
> [PMD: Add missing "qemu/error-report.h" include and TODO comment]
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   hw/block/pflash_cfi01.c | 8 +++++++-
>   hw/block/pflash_cfi02.c | 8 +++++++-
>   2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/pflash_cfi01.c b/hw/block/pflash_cfi01.c
> index 24f3bce7ef..be1954c5d8 100644
> --- a/hw/block/pflash_cfi01.c
> +++ b/hw/block/pflash_cfi01.c
> @@ -42,6 +42,7 @@
>   #include "hw/qdev-properties.h"
>   #include "sysemu/block-backend.h"
>   #include "qapi/error.h"
> +#include "qemu/error-report.h"
>   #include "qemu/timer.h"
>   #include "qemu/bitops.h"
>   #include "qemu/error-report.h"
> @@ -399,13 +400,18 @@ static void pflash_update(PFlashCFI01 *pfl, int offset,
>                             int size)
>   {
>       int offset_end;
> +    int ret;
>       if (pfl->blk) {
>           offset_end = offset + size;
>           /* widen to sector boundaries */
>           offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
>           offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> -        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
> +        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
>                      offset_end - offset, 0);
> +        if (ret < 0) {
> +            /* TODO set error bit in status */
> +            error_report("Could not update PFLASH: %s", strerror(-ret));
> +        }
>       }
>   }
>   
> diff --git a/hw/block/pflash_cfi02.c b/hw/block/pflash_cfi02.c
> index 12f18d401a..c6b6f2d082 100644
> --- a/hw/block/pflash_cfi02.c
> +++ b/hw/block/pflash_cfi02.c
> @@ -37,6 +37,7 @@
>   #include "hw/block/flash.h"
>   #include "hw/qdev-properties.h"
>   #include "qapi/error.h"
> +#include "qemu/error-report.h"
>   #include "qemu/bitmap.h"
>   #include "qemu/timer.h"
>   #include "sysemu/block-backend.h"
> @@ -393,13 +394,18 @@ static uint64_t pflash_read(void *opaque, hwaddr offset, unsigned int width)
>   static void pflash_update(PFlashCFI02 *pfl, int offset, int size)
>   {
>       int offset_end;
> +    int ret;
>       if (pfl->blk) {
>           offset_end = offset + size;
>           /* widen to sector boundaries */
>           offset = QEMU_ALIGN_DOWN(offset, BDRV_SECTOR_SIZE);
>           offset_end = QEMU_ALIGN_UP(offset_end, BDRV_SECTOR_SIZE);
> -        blk_pwrite(pfl->blk, offset, pfl->storage + offset,
> +        ret = blk_pwrite(pfl->blk, offset, pfl->storage + offset,
>                      offset_end - offset, 0);
> +        if (ret < 0) {
> +            /* TODO set error bit in status */
> +            error_report("Could not update PFLASH: %s", strerror(-ret));
> +        }
>       }
>   }
>   
> 

Re: [PATCH-for-5.0 04/12] qga: Restrict guest-file-read count to 48 MB to avoid crashes

[Daniel P. Berrangé]

On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote:
> On [*] Daniel Berrangé commented:
> 
>   The QEMU guest agent protocol is not sensible way to access huge
>   files inside the guest. It requires the inefficient process of
>   reading the entire data into memory than duplicating it again in
>   base64 format, and then copying it again in the JSON serializer /
>   monitor code.
> 
>   For arbitrary general purpose file access, especially for large
>   files, use a real file transfer program or use a network block
>   device, not the QEMU guest agent.
> 
> To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his
> suggestion to put a low, hard limit on "count" in the guest agent
> QAPI schema, and don't allow count to be larger than 48 MB.
> 
> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
> 
> Fixes: CVE-2018-12617
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
> Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  qga/qapi-schema.json | 6 ++++--
>  qga/commands.c       | 9 ++++++++-
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> index f6fcb59f34..7758d9daf8 100644
> --- a/qga/qapi-schema.json
> +++ b/qga/qapi-schema.json
> @@ -266,11 +266,13 @@
>  ##
>  # @guest-file-read:
>  #
> -# Read from an open file in the guest. Data will be base64-encoded
> +# Read from an open file in the guest. Data will be base64-encoded.
> +# As this command is just for limited, ad-hoc debugging, such as log
> +# file access, the number of bytes to read is limited to 10 MB.

s/10/48/

>  #
>  # @handle: filehandle returned by guest-file-open
>  #
> -# @count: maximum number of bytes to read (default is 4KB)
> +# @count: maximum number of bytes to read (default is 4KB, maximum is 10MB)

s/10/48/

>  #
>  # Returns: @GuestFileRead on success.
>  #
> diff --git a/qga/commands.c b/qga/commands.c
> index 5611117372..efc8b90281 100644
> --- a/qga/commands.c
> +++ b/qga/commands.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/units.h"
>  #include "guest-agent-core.h"
>  #include "qga-qapi-commands.h"
>  #include "qapi/error.h"
> @@ -24,6 +25,12 @@
>  #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
>  /* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
>  #define GUEST_EXEC_IO_SIZE (4*1024)
> +/*
> + * Maximum file size to read - 48MB
> + *
> + * (48MB + Base64 3:4 overhead = JSON parser 64 MB limit)
> + */
> +#define GUEST_FILE_READ_COUNT_MAX (48 * MiB)
>  
>  /* Note: in some situations, like with the fsfreeze, logging may be
>   * temporarilly disabled. if it is necessary that a command be able
> @@ -560,7 +567,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>      }
>      if (!has_count) {
>          count = QGA_READ_COUNT_DEFAULT;
> -    } else if (count < 0 || count >= UINT32_MAX) {
> +    } else if (count < 0 || count > GUEST_FILE_READ_COUNT_MAX) {
>          error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
>                     count);
>          return NULL;

With the docs typos fixed:

  Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH-for-5.0 04/12] qga: Restrict guest-file-read count to 48 MB to avoid crashes

[Philippe Mathieu-Daudé]

On 4/15/20 2:34 PM, Daniel P. Berrangé wrote:
> On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote:
>> On [*] Daniel Berrangé commented:
>>
>>    The QEMU guest agent protocol is not sensible way to access huge
>>    files inside the guest. It requires the inefficient process of
>>    reading the entire data into memory than duplicating it again in
>>    base64 format, and then copying it again in the JSON serializer /
>>    monitor code.
>>
>>    For arbitrary general purpose file access, especially for large
>>    files, use a real file transfer program or use a network block
>>    device, not the QEMU guest agent.
>>
>> To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his
>> suggestion to put a low, hard limit on "count" in the guest agent
>> QAPI schema, and don't allow count to be larger than 48 MB.
>>
>> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
>>
>> Fixes: CVE-2018-12617
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
>> Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
>> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   qga/qapi-schema.json | 6 ++++--
>>   qga/commands.c       | 9 ++++++++-
>>   2 files changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
>> index f6fcb59f34..7758d9daf8 100644
>> --- a/qga/qapi-schema.json
>> +++ b/qga/qapi-schema.json
>> @@ -266,11 +266,13 @@
>>   ##
>>   # @guest-file-read:
>>   #
>> -# Read from an open file in the guest. Data will be base64-encoded
>> +# Read from an open file in the guest. Data will be base64-encoded.
>> +# As this command is just for limited, ad-hoc debugging, such as log
>> +# file access, the number of bytes to read is limited to 10 MB.
> 
> s/10/48/
> 
>>   #
>>   # @handle: filehandle returned by guest-file-open
>>   #
>> -# @count: maximum number of bytes to read (default is 4KB)
>> +# @count: maximum number of bytes to read (default is 4KB, maximum is 10MB)
> 
> s/10/48/

Oops I totally missed those, thanks!

> 
>>   #
>>   # Returns: @GuestFileRead on success.
>>   #
>> diff --git a/qga/commands.c b/qga/commands.c
>> index 5611117372..efc8b90281 100644
>> --- a/qga/commands.c
>> +++ b/qga/commands.c
>> @@ -11,6 +11,7 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "qemu/units.h"
>>   #include "guest-agent-core.h"
>>   #include "qga-qapi-commands.h"
>>   #include "qapi/error.h"
>> @@ -24,6 +25,12 @@
>>   #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
>>   /* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
>>   #define GUEST_EXEC_IO_SIZE (4*1024)
>> +/*
>> + * Maximum file size to read - 48MB
>> + *
>> + * (48MB + Base64 3:4 overhead = JSON parser 64 MB limit)
>> + */
>> +#define GUEST_FILE_READ_COUNT_MAX (48 * MiB)
>>   
>>   /* Note: in some situations, like with the fsfreeze, logging may be
>>    * temporarilly disabled. if it is necessary that a command be able
>> @@ -560,7 +567,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
>>       }
>>       if (!has_count) {
>>           count = QGA_READ_COUNT_DEFAULT;
>> -    } else if (count < 0 || count >= UINT32_MAX) {
>> +    } else if (count < 0 || count > GUEST_FILE_READ_COUNT_MAX) {
>>           error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
>>                      count);
>>           return NULL;
> 
> With the docs typos fixed:
> 
>    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> 
> Regards,
> Daniel
> 

Re: [PATCH-for-5.0 04/12] qga: Restrict guest-file-read count to 48 MB to avoid crashes

[Michael Roth]

Quoting Philippe Mathieu-Daudé (2020-04-15 08:02:18)
> On 4/15/20 2:34 PM, Daniel P. Berrangé wrote:
> > On Tue, Apr 14, 2020 at 03:30:44PM +0200, Philippe Mathieu-Daudé wrote:
> >> On [*] Daniel Berrangé commented:
> >>
> >>    The QEMU guest agent protocol is not sensible way to access huge
> >>    files inside the guest. It requires the inefficient process of
> >>    reading the entire data into memory than duplicating it again in
> >>    base64 format, and then copying it again in the JSON serializer /
> >>    monitor code.
> >>
> >>    For arbitrary general purpose file access, especially for large
> >>    files, use a real file transfer program or use a network block
> >>    device, not the QEMU guest agent.
> >>
> >> To avoid bug reports as BZ#1594054 (CVE-2018-12617), follow his
> >> suggestion to put a low, hard limit on "count" in the guest agent
> >> QAPI schema, and don't allow count to be larger than 48 MB.
> >>
> >> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg693176.html
> >>
> >> Fixes: CVE-2018-12617
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1594054
> >> Reported-by: Fakhri Zulkifli <mohdfakhrizulkifli@gmail.com>
> >> Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> ---
> >>   qga/qapi-schema.json | 6 ++++--
> >>   qga/commands.c       | 9 ++++++++-
> >>   2 files changed, 12 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
> >> index f6fcb59f34..7758d9daf8 100644
> >> --- a/qga/qapi-schema.json
> >> +++ b/qga/qapi-schema.json
> >> @@ -266,11 +266,13 @@
> >>   ##
> >>   # @guest-file-read:
> >>   #
> >> -# Read from an open file in the guest. Data will be base64-encoded
> >> +# Read from an open file in the guest. Data will be base64-encoded.
> >> +# As this command is just for limited, ad-hoc debugging, such as log
> >> +# file access, the number of bytes to read is limited to 10 MB.
> > 
> > s/10/48/
> > 
> >>   #
> >>   # @handle: filehandle returned by guest-file-open
> >>   #
> >> -# @count: maximum number of bytes to read (default is 4KB)
> >> +# @count: maximum number of bytes to read (default is 4KB, maximum is 10MB)
> > 
> > s/10/48/
> 
> Oops I totally missed those, thanks!

I've rolled in the doc fix-ups and sent a pull request for patches 1-4

> 
> > 
> >>   #
> >>   # Returns: @GuestFileRead on success.
> >>   #
> >> diff --git a/qga/commands.c b/qga/commands.c
> >> index 5611117372..efc8b90281 100644
> >> --- a/qga/commands.c
> >> +++ b/qga/commands.c
> >> @@ -11,6 +11,7 @@
> >>    */
> >>   
> >>   #include "qemu/osdep.h"
> >> +#include "qemu/units.h"
> >>   #include "guest-agent-core.h"
> >>   #include "qga-qapi-commands.h"
> >>   #include "qapi/error.h"
> >> @@ -24,6 +25,12 @@
> >>   #define GUEST_EXEC_MAX_OUTPUT (16*1024*1024)
> >>   /* Allocation and I/O buffer for reading guest-exec out_data/err_data - 4KB */
> >>   #define GUEST_EXEC_IO_SIZE (4*1024)
> >> +/*
> >> + * Maximum file size to read - 48MB
> >> + *
> >> + * (48MB + Base64 3:4 overhead = JSON parser 64 MB limit)
> >> + */
> >> +#define GUEST_FILE_READ_COUNT_MAX (48 * MiB)
> >>   
> >>   /* Note: in some situations, like with the fsfreeze, logging may be
> >>    * temporarilly disabled. if it is necessary that a command be able
> >> @@ -560,7 +567,7 @@ GuestFileRead *qmp_guest_file_read(int64_t handle, bool has_count,
> >>       }
> >>       if (!has_count) {
> >>           count = QGA_READ_COUNT_DEFAULT;
> >> -    } else if (count < 0 || count >= UINT32_MAX) {
> >> +    } else if (count < 0 || count > GUEST_FILE_READ_COUNT_MAX) {
> >>           error_setg(errp, "value '%" PRId64 "' is invalid for argument count",
> >>                      count);
> >>           return NULL;
> > 
> > With the docs typos fixed:
> > 
> >    Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > 
> > 
> > Regards,
> > Daniel
> > 
> 

Re: [PATCH-for-5.0 05/12] vhost-user-gpu: Release memory returned by vu_queue_pop() with free()

[Michael S. Tsirkin]

On Tue, Apr 14, 2020 at 03:30:45PM +0200, Philippe Mathieu-Daudé wrote:
> vu_queue_pop() returns memory that must be freed with free().
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: Coverity (CID 1421887 ALLOC_FREE_MISMATCH)
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  contrib/vhost-user-gpu/vhost-user-gpu.c | 4 ++--
>  contrib/vhost-user-gpu/virgl.c          | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/vhost-user-gpu/vhost-user-gpu.c b/contrib/vhost-user-gpu/vhost-user-gpu.c
> index b45d2019b4..a019d0a9ac 100644
> --- a/contrib/vhost-user-gpu/vhost-user-gpu.c
> +++ b/contrib/vhost-user-gpu/vhost-user-gpu.c
> @@ -848,7 +848,7 @@ vg_handle_ctrl(VuDev *dev, int qidx)
>              QTAILQ_INSERT_TAIL(&vg->fenceq, cmd, next);
>              vg->inflight++;
>          } else {
> -            g_free(cmd);
> +            free(cmd);
>          }
>      }
>  }
> @@ -939,7 +939,7 @@ vg_handle_cursor(VuDev *dev, int qidx)
>          }
>          vu_queue_push(dev, vq, elem, 0);
>          vu_queue_notify(dev, vq);
> -        g_free(elem);
> +        free(elem);
>      }
>  }
>  
> diff --git a/contrib/vhost-user-gpu/virgl.c b/contrib/vhost-user-gpu/virgl.c
> index 43413e29df..b0bc22c3c1 100644
> --- a/contrib/vhost-user-gpu/virgl.c
> +++ b/contrib/vhost-user-gpu/virgl.c
> @@ -519,7 +519,7 @@ virgl_write_fence(void *opaque, uint32_t fence)
>          g_debug("FENCE %" PRIu64, cmd->cmd_hdr.fence_id);
>          vg_ctrl_response_nodata(g, cmd, VIRTIO_GPU_RESP_OK_NODATA);
>          QTAILQ_REMOVE(&g->fenceq, cmd, next);
> -        g_free(cmd);
> +        free(cmd);
>          g->inflight--;
>      }
>  }
> -- 
> 2.21.1

Re: [PATCH-for-5.0 00/12] various bugfixes

[Michael S. Tsirkin]

On Tue, Apr 14, 2020 at 03:30:40PM +0200, Philippe Mathieu-Daudé wrote:
> Collection of bugfixes for 5.0.
> 
> Only vhost-user-gpu/grlib_ahb_apb_pnp patches are reviewed.
> 
> As 5.0-rc3 is tomorrow, I thought it could help to gather
> them and resend altogether.


So who's applying all this stuff? Peter?

> Regards,
> 
> Phil.
> 
> Mansour Ahmadi (1):
>   hw/block/pflash: Check return value of blk_pwrite()
> 
> Philippe Mathieu-Daudé (11):
>   Revert "prevent crash when executing guest-file-read with large count"
>   qga: Extract guest_file_handle_find() to commands-common.h
>   qga: Extract qmp_guest_file_read() to common commands.c
>   qga: Restrict guest-file-read count to 48 MB to avoid crashes
>   vhost-user-gpu: Release memory returned by vu_queue_pop() with free()
>   hw/openrisc/pic_cpu: Use qdev gpio rather than qemu_allocate_irqs()
>   hw/misc/grlib_ahb_apb_pnp: Avoid crash when writing to AHB PnP
>     registers
>   hw/misc/grlib_ahb_apb_pnp: Fix AHB PnP 8-bit accesses
>   hw/display/sm501: Avoid heap overflow in sm501_2d_operation()
>   gdbstub: Do not use memset() on GByteArray
>   gdbstub: Introduce gdb_get_freg32() to get float32 registers
> 
>  qga/qapi-schema.json                    |  6 +++--
>  include/exec/gdbstub.h                  | 22 ++++++++++++++++
>  qga/commands-common.h                   | 21 +++++++++++++++
>  contrib/vhost-user-gpu/vhost-user-gpu.c |  4 +--
>  contrib/vhost-user-gpu/virgl.c          |  2 +-
>  hw/block/pflash_cfi01.c                 |  8 +++++-
>  hw/block/pflash_cfi02.c                 |  8 +++++-
>  hw/display/sm501.c                      |  6 +++++
>  hw/misc/grlib_ahb_apb_pnp.c             | 11 ++++++++
>  hw/openrisc/pic_cpu.c                   |  5 ++--
>  qga/commands-posix.c                    | 29 +++++---------------
>  qga/commands-win32.c                    | 35 ++++++-------------------
>  qga/commands.c                          | 33 +++++++++++++++++++++++
>  target/arm/gdbstub.c                    |  3 +--
>  target/sh4/gdbstub.c                    |  6 ++---
>  target/xtensa/gdbstub.c                 |  6 ++---
>  16 files changed, 136 insertions(+), 69 deletions(-)
>  create mode 100644 qga/commands-common.h
> 
> -- 
> 2.21.1

Re: [PATCH-for-5.0 00/12] various bugfixes

[Peter Maydell]

On Fri, 17 Apr 2020 at 07:40, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Apr 14, 2020 at 03:30:40PM +0200, Philippe Mathieu-Daudé wrote:
> > Collection of bugfixes for 5.0.
> >
> > Only vhost-user-gpu/grlib_ahb_apb_pnp patches are reviewed.
> >
> > As 5.0-rc3 is tomorrow, I thought it could help to gather
> > them and resend altogether.
>
>
> So who's applying all this stuff? Peter?

I talked to Philippe about this on irc; I cherry picked the
one bug that we thought was critical for 5.0 (the vhost-user-gpu
free/g_free mismatch bugfix) and the rest will be for 5.1.

thanks
-- PMM