[RFC PATCH-for-7.2 0/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()

[RFC PATCH-for-7.2 0/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()

[Philippe Mathieu-Daudé]

memory_region_get_ram_ptr() returns a host pointer for a
MemoryRegion. Sometimes we do offset calculation using this
pointer without checking the underlying MemoryRegion size.

Wenxu Yin reported a buffer overrun in QXL. This series
aims to fix it. I haven't audited the other _get_ram_ptr()
uses (yet). Eventually we could rename it _get_ram_ptr_unsafe
and add a safer helper which checks for overrun.

Worth considering for 7.2?

Regards,

Phil.

Philippe Mathieu-Daudé (4):
  hw/display/qxl: Have qxl_log_command Return early if no log_cmd
    handler
  hw/display/qxl: Document qxl_phys2virt()
  hw/display/qxl: Pass qxl_phys2virt size
  hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()

 hw/display/qxl-logger.c | 22 +++++++++++++++++++---
 hw/display/qxl-render.c | 11 +++++++----
 hw/display/qxl.c        | 25 +++++++++++++++++++------
 hw/display/qxl.h        | 23 ++++++++++++++++++++++-
 4 files changed, 67 insertions(+), 14 deletions(-)

-- 
2.38.1

[RFC PATCH-for-7.2 1/4] hw/display/qxl: Have qxl_log_command Return early if no log_cmd handler

[Philippe Mathieu-Daudé]

Only 3 command types are logged: no need to call qxl_phys2virt()
for the other types.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/qxl-logger.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index 68bfa47568..1bcf803db6 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -247,6 +247,16 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
             qxl_name(qxl_type, ext->cmd.type),
             compat ? "(compat)" : "");
 
+    switch (ext->cmd.type) {
+    case QXL_CMD_DRAW:
+        break;
+    case QXL_CMD_SURFACE:
+        break;
+    case QXL_CMD_CURSOR:
+        break;
+    default:
+        goto out;
+    }
     data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
     if (!data) {
         return 1;
@@ -269,6 +279,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
         qxl_log_cmd_cursor(qxl, data, ext->group_id);
         break;
     }
+out:
     fprintf(stderr, "\n");
     return 0;
 }
-- 
2.38.1

[RFC PATCH-for-7.2 2/4] hw/display/qxl: Document qxl_phys2virt()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/qxl.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index e74de9579d..78b3a6c9ba 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -147,6 +147,25 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
 #define QXL_DEFAULT_REVISION (QXL_REVISION_STABLE_V12 + 1)
 
 /* qxl.c */
+/**
+ * qxl_phys2virt: Get a pointer within a PCI VRAM memory region.
+ *
+ * @qxl: QXL device
+ * @phys: physical offset of buffer within the VRAM
+ * @group_id: memory slot group
+ *
+ * Returns a host pointer to a buffer placed at offset @phys within the
+ * active slot @group_id of the PCI VGA RAM memory region associated with
+ * the @qxl device. If the slot is inactive, or the offset is out
+ * of the memory region, returns NULL.
+ *
+ * Use with care; by the time this function returns, the returned pointer is
+ * not protected by RCU anymore.  If the caller is not within an RCU critical
+ * section and does not hold the iothread lock, it must have other means of
+ * protecting the pointer, such as a reference to the region that includes
+ * the incoming ram_addr_t.
+ *
+ */
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
 void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
     G_GNUC_PRINTF(2, 3);
-- 
2.38.1

Re: [RFC PATCH-for-7.2 0/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()

[Mauro Matteo Cascella]

On Fri, Nov 25, 2022 at 4:40 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> memory_region_get_ram_ptr() returns a host pointer for a
> MemoryRegion. Sometimes we do offset calculation using this
> pointer without checking the underlying MemoryRegion size.
>
> Wenxu Yin reported a buffer overrun in QXL. This series
> aims to fix it. I haven't audited the other _get_ram_ptr()
> uses (yet). Eventually we could rename it _get_ram_ptr_unsafe
> and add a safer helper which checks for overrun.

This is now CVE-2022-4144. Please add proper "Fixes:" tag, if possible.

Thank you for the fix.

> Worth considering for 7.2?
>
> Regards,
>
> Phil.
>
> Philippe Mathieu-Daudé (4):
>   hw/display/qxl: Have qxl_log_command Return early if no log_cmd
>     handler
>   hw/display/qxl: Document qxl_phys2virt()
>   hw/display/qxl: Pass qxl_phys2virt size
>   hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()
>
>  hw/display/qxl-logger.c | 22 +++++++++++++++++++---
>  hw/display/qxl-render.c | 11 +++++++----
>  hw/display/qxl.c        | 25 +++++++++++++++++++------
>  hw/display/qxl.h        | 23 ++++++++++++++++++++++-
>  4 files changed, 67 insertions(+), 14 deletions(-)
>
> --
> 2.38.1
>


-- 
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0

[RFC PATCH-for-7.2 3/4] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()

[Philippe Mathieu-Daudé]

Currently qxl_phys2virt() doesn't check for buffer overrun.
In order to do so in the next commit, pass the buffer size
as argument.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
RFC: Please double-check qxl_render_update_area_unlocked()
---
 hw/display/qxl-logger.c | 11 ++++++++---
 hw/display/qxl-render.c | 11 +++++++----
 hw/display/qxl.c        | 14 +++++++++-----
 hw/display/qxl.h        |  4 +++-
 4 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
index 1bcf803db6..35c38f6252 100644
--- a/hw/display/qxl-logger.c
+++ b/hw/display/qxl-logger.c
@@ -106,7 +106,7 @@ static int qxl_log_image(PCIQXLDevice *qxl, QXLPHYSICAL addr, int group_id)
     QXLImage *image;
     QXLImageDescriptor *desc;
 
-    image = qxl_phys2virt(qxl, addr, group_id);
+    image = qxl_phys2virt(qxl, addr, group_id, sizeof(QXLImage));
     if (!image) {
         return 1;
     }
@@ -214,7 +214,8 @@ int qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id)
                 cmd->u.set.position.y,
                 cmd->u.set.visible ? "yes" : "no",
                 cmd->u.set.shape);
-        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id);
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id,
+                               sizeof(QXLCursor));
         if (!cursor) {
             return 1;
         }
@@ -236,6 +237,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
 {
     bool compat = ext->flags & QXL_COMMAND_FLAG_COMPAT;
     void *data;
+    size_t datasz;
     int ret;
 
     if (!qxl->cmdlog) {
@@ -249,15 +251,18 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
 
     switch (ext->cmd.type) {
     case QXL_CMD_DRAW:
+        datasz = compat ? sizeof(QXLCompatDrawable) : sizeof(QXLDrawable);
         break;
     case QXL_CMD_SURFACE:
+        datasz = sizeof(QXLSurfaceCmd);
         break;
     case QXL_CMD_CURSOR:
+        datasz = sizeof(QXLCursorCmd);
         break;
     default:
         goto out;
     }
-    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id, datasz);
     if (!data) {
         return 1;
     }
diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index ca217004bf..1b0a50c1aa 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -107,7 +107,8 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
         qxl->guest_primary.resized = 0;
         qxl->guest_primary.data = qxl_phys2virt(qxl,
                                                 qxl->guest_primary.surface.mem,
-                                                MEMSLOT_GROUP_GUEST);
+                                                MEMSLOT_GROUP_GUEST,
+                                                sizeof(uint32_t) * width * height);
         if (!qxl->guest_primary.data) {
             goto end;
         }
@@ -228,7 +229,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
         if (offset == size) {
             return;
         }
-        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
+        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
         if (!chunk) {
             return;
         }
@@ -295,7 +296,8 @@ fail:
 /* called from spice server thread context only */
 int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
 {
-    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                      sizeof(QXLCursorCmd));
     QXLCursor *cursor;
     QEMUCursor *c;
 
@@ -314,7 +316,8 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
     }
     switch (cmd->type) {
     case QXL_CURSOR_SET:
-        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
+        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id,
+                               sizeof(QXLCursor));
         if (!cursor) {
             return 1;
         }
diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 5b10f697f1..231d733250 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -274,7 +274,8 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
                                           QXL_IO_MONITORS_CONFIG_ASYNC));
     }
 
-    cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config, MEMSLOT_GROUP_GUEST);
+    cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config, MEMSLOT_GROUP_GUEST,
+                        sizeof(QXLMonitorsConfig));
     if (cfg != NULL && cfg->count == 1) {
         qxl->guest_primary.resized = 1;
         qxl->guest_head0_width  = cfg->heads[0].width;
@@ -459,7 +460,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
     switch (le32_to_cpu(ext->cmd.type)) {
     case QXL_CMD_SURFACE:
     {
-        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                           sizeof(QXLSurfaceCmd));
 
         if (!cmd) {
             return 1;
@@ -494,7 +496,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
     }
     case QXL_CMD_CURSOR:
     {
-        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
+        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
+                                          sizeof(QXLCursorCmd));
 
         if (!cmd) {
             return 1;
@@ -1456,7 +1459,8 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
 }
 
 /* can be also called from spice server thread context */
-void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
+                    size_t size)
 {
     uint64_t offset;
     uint32_t slot;
@@ -1964,7 +1968,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
         }
 
         cmd = qxl_phys2virt(qxl, qxl->guest_surfaces.cmds[i],
-                            MEMSLOT_GROUP_GUEST);
+                            MEMSLOT_GROUP_GUEST, sizeof(QXLSurfaceCmd));
         assert(cmd);
         assert(cmd->type == QXL_SURFACE_CMD_CREATE);
         qxl_dirty_one_surface(qxl, cmd->u.surface_create.data,
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index 78b3a6c9ba..bf03138ab4 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -153,6 +153,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
  * @qxl: QXL device
  * @phys: physical offset of buffer within the VRAM
  * @group_id: memory slot group
+ * @size: size of the buffer
  *
  * Returns a host pointer to a buffer placed at offset @phys within the
  * active slot @group_id of the PCI VGA RAM memory region associated with
@@ -166,7 +167,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
  * the incoming ram_addr_t.
  *
  */
-void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
+void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id,
+                    size_t size);
 void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
     G_GNUC_PRINTF(2, 3);
 
-- 
2.38.1

[RFC PATCH-for-7.2 4/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144)

[Philippe Mathieu-Daudé]

Return NULL if the requested buffer size does not fit
within the slot memory region.

Reported-by: Wenxu Yin (@awxylitol)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1336
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 hw/display/qxl.c | 11 ++++++++++-
 hw/display/qxl.h |  2 +-
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index 231d733250..e5e162f82d 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -1462,7 +1462,7 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
 void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
                     size_t size)
 {
-    uint64_t offset;
+    uint64_t offset, ptr_end_offset;
     uint32_t slot;
     void *ptr;
 
@@ -1474,6 +1474,15 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
         if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset)) {
             return NULL;
         }
+
+        ptr_end_offset = qxl->guest_slots[slot].offset + offset + size;
+        if (ptr_end_offset > memory_region_size(qxl->guest_slots[slot].mr)) {
+            qxl_set_guest_bug(qxl,
+                              "slot %d offset %"PRIu64" size %zu: "
+                              "overrun by %"PRIu64" bytes\n",
+                              slot, offset, size, ptr_end_offset - offset);
+            return NULL;
+        }
         ptr = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr);
         ptr += qxl->guest_slots[slot].offset;
         ptr += offset;
diff --git a/hw/display/qxl.h b/hw/display/qxl.h
index bf03138ab4..7894bd5134 100644
--- a/hw/display/qxl.h
+++ b/hw/display/qxl.h
@@ -157,7 +157,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
  *
  * Returns a host pointer to a buffer placed at offset @phys within the
  * active slot @group_id of the PCI VGA RAM memory region associated with
- * the @qxl device. If the slot is inactive, or the offset is out
+ * the @qxl device. If the slot is inactive, or the offset + size are out
  * of the memory region, returns NULL.
  *
  * Use with care; by the time this function returns, the returned pointer is
-- 
2.38.1

Re: [RFC PATCH-for-7.2 0/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()

[Philippe Mathieu-Daudé]

> Philippe Mathieu-Daudé (4):
>    hw/display/qxl: Have qxl_log_command Return early if no log_cmd
>      handler
>    hw/display/qxl: Document qxl_phys2virt()
>    hw/display/qxl: Pass qxl_phys2virt size
>    hw/display/qxl: Avoid buffer overrun in qxl_phys2virt()
> 
>   hw/display/qxl-logger.c | 22 +++++++++++++++++++---
>   hw/display/qxl-render.c | 11 +++++++----
>   hw/display/qxl.c        | 25 +++++++++++++++++++------
>   hw/display/qxl.h        | 23 ++++++++++++++++++++++-
>   4 files changed, 67 insertions(+), 14 deletions(-)

I am having hard time with my MTA:

   4.3.0 Temporary System Problem.  Try again later (2). 
k1-20020a7bc401000000b003cfbe1da539sm5571640wmi.36 - gsmtp

Sorry if this series is mis-posted, I'll try to resend as a
whole later.

Re: [RFC PATCH-for-7.2 3/4] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()

[Marc-André Lureau]

On Fri, Nov 25, 2022 at 9:35 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Currently qxl_phys2virt() doesn't check for buffer overrun.
> In order to do so in the next commit, pass the buffer size
> as argument.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>



> ---
> RFC: Please double-check qxl_render_update_area_unlocked()
> ---
>  hw/display/qxl-logger.c | 11 ++++++++---
>  hw/display/qxl-render.c | 11 +++++++----
>  hw/display/qxl.c        | 14 +++++++++-----
>  hw/display/qxl.h        |  4 +++-
>  4 files changed, 27 insertions(+), 13 deletions(-)
>
> diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
> index 1bcf803db6..35c38f6252 100644
> --- a/hw/display/qxl-logger.c
> +++ b/hw/display/qxl-logger.c
> @@ -106,7 +106,7 @@ static int qxl_log_image(PCIQXLDevice *qxl, QXLPHYSICAL addr, int group_id)
>      QXLImage *image;
>      QXLImageDescriptor *desc;
>
> -    image = qxl_phys2virt(qxl, addr, group_id);
> +    image = qxl_phys2virt(qxl, addr, group_id, sizeof(QXLImage));
>      if (!image) {
>          return 1;
>      }
> @@ -214,7 +214,8 @@ int qxl_log_cmd_cursor(PCIQXLDevice *qxl, QXLCursorCmd *cmd, int group_id)
>                  cmd->u.set.position.y,
>                  cmd->u.set.visible ? "yes" : "no",
>                  cmd->u.set.shape);
> -        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id);
> +        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, group_id,
> +                               sizeof(QXLCursor));
>          if (!cursor) {
>              return 1;
>          }
> @@ -236,6 +237,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
>  {
>      bool compat = ext->flags & QXL_COMMAND_FLAG_COMPAT;
>      void *data;
> +    size_t datasz;
>      int ret;
>
>      if (!qxl->cmdlog) {
> @@ -249,15 +251,18 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
>
>      switch (ext->cmd.type) {
>      case QXL_CMD_DRAW:
> +        datasz = compat ? sizeof(QXLCompatDrawable) : sizeof(QXLDrawable);
>          break;
>      case QXL_CMD_SURFACE:
> +        datasz = sizeof(QXLSurfaceCmd);
>          break;
>      case QXL_CMD_CURSOR:
> +        datasz = sizeof(QXLCursorCmd);
>          break;
>      default:
>          goto out;
>      }
> -    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +    data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id, datasz);
>      if (!data) {
>          return 1;
>      }
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index ca217004bf..1b0a50c1aa 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -107,7 +107,8 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
>          qxl->guest_primary.resized = 0;
>          qxl->guest_primary.data = qxl_phys2virt(qxl,
>                                                  qxl->guest_primary.surface.mem,
> -                                                MEMSLOT_GROUP_GUEST);
> +                                                MEMSLOT_GROUP_GUEST,
> +                                                sizeof(uint32_t) * width * height);

It looks wrong, I think it should be:

qxl->guest_primary.abs_stride * height * qxl->guest_primary.bytes_pp

>          if (!qxl->guest_primary.data) {
>              goto end;
>          }
> @@ -228,7 +229,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>          if (offset == size) {
>              return;
>          }
> -        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id);
> +        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id, bytes);
>          if (!chunk) {
>              return;
>          }
> @@ -295,7 +296,8 @@ fail:
>  /* called from spice server thread context only */
>  int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
>  {
> -    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +    QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
> +                                      sizeof(QXLCursorCmd));
>      QXLCursor *cursor;
>      QEMUCursor *c;
>
> @@ -314,7 +316,8 @@ int qxl_render_cursor(PCIQXLDevice *qxl, QXLCommandExt *ext)
>      }
>      switch (cmd->type) {
>      case QXL_CURSOR_SET:
> -        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id);
> +        cursor = qxl_phys2virt(qxl, cmd->u.set.shape, ext->group_id,
> +                               sizeof(QXLCursor));
>          if (!cursor) {
>              return 1;
>          }
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 5b10f697f1..231d733250 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -274,7 +274,8 @@ static void qxl_spice_monitors_config_async(PCIQXLDevice *qxl, int replay)
>                                            QXL_IO_MONITORS_CONFIG_ASYNC));
>      }
>
> -    cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config, MEMSLOT_GROUP_GUEST);
> +    cfg = qxl_phys2virt(qxl, qxl->guest_monitors_config, MEMSLOT_GROUP_GUEST,
> +                        sizeof(QXLMonitorsConfig));
>      if (cfg != NULL && cfg->count == 1) {
>          qxl->guest_primary.resized = 1;
>          qxl->guest_head0_width  = cfg->heads[0].width;
> @@ -459,7 +460,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
>      switch (le32_to_cpu(ext->cmd.type)) {
>      case QXL_CMD_SURFACE:
>      {
> -        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +        QXLSurfaceCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
> +                                           sizeof(QXLSurfaceCmd));
>
>          if (!cmd) {
>              return 1;
> @@ -494,7 +496,8 @@ static int qxl_track_command(PCIQXLDevice *qxl, struct QXLCommandExt *ext)
>      }
>      case QXL_CMD_CURSOR:
>      {
> -        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);
> +        QXLCursorCmd *cmd = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id,
> +                                          sizeof(QXLCursorCmd));
>
>          if (!cmd) {
>              return 1;
> @@ -1456,7 +1459,8 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
>  }
>
>  /* can be also called from spice server thread context */
> -void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id)
> +void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
> +                    size_t size)
>  {
>      uint64_t offset;
>      uint32_t slot;
> @@ -1964,7 +1968,7 @@ static void qxl_dirty_surfaces(PCIQXLDevice *qxl)
>          }
>
>          cmd = qxl_phys2virt(qxl, qxl->guest_surfaces.cmds[i],
> -                            MEMSLOT_GROUP_GUEST);
> +                            MEMSLOT_GROUP_GUEST, sizeof(QXLSurfaceCmd));
>          assert(cmd);
>          assert(cmd->type == QXL_SURFACE_CMD_CREATE);
>          qxl_dirty_one_surface(qxl, cmd->u.surface_create.data,
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index 78b3a6c9ba..bf03138ab4 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -153,6 +153,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
>   * @qxl: QXL device
>   * @phys: physical offset of buffer within the VRAM
>   * @group_id: memory slot group
> + * @size: size of the buffer
>   *
>   * Returns a host pointer to a buffer placed at offset @phys within the
>   * active slot @group_id of the PCI VGA RAM memory region associated with
> @@ -166,7 +167,8 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
>   * the incoming ram_addr_t.
>   *
>   */
> -void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
> +void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id,
> +                    size_t size);
>  void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
>      G_GNUC_PRINTF(2, 3);
>
> --
> 2.38.1
>
>


-- 
Marc-André Lureau

Re: [RFC PATCH-for-7.2 1/4] hw/display/qxl: Have qxl_log_command Return early if no log_cmd handler

[Marc-André Lureau]

Hi

On Fri, Nov 25, 2022 at 7:41 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Only 3 command types are logged: no need to call qxl_phys2virt()
> for the other types.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/display/qxl-logger.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/hw/display/qxl-logger.c b/hw/display/qxl-logger.c
> index 68bfa47568..1bcf803db6 100644
> --- a/hw/display/qxl-logger.c
> +++ b/hw/display/qxl-logger.c
> @@ -247,6 +247,16 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
>              qxl_name(qxl_type, ext->cmd.type),
>              compat ? "(compat)" : "");
>
> +    switch (ext->cmd.type) {
> +    case QXL_CMD_DRAW:
> +        break;
> +    case QXL_CMD_SURFACE:
> +        break;
> +    case QXL_CMD_CURSOR:
> +        break;
> +    default:
> +        goto out;
> +    }

That's a quite verbose way to repeat the case list below. Furthermore,
it shouldn't hurt to call qxl_phys2virt() next.


>      data = qxl_phys2virt(qxl, ext->cmd.data, ext->group_id);

But I understand better the change looking at patch 3, where you
introduce the size argument. Maybe mention the coming change in the
commit message?


Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>      if (!data) {
>          return 1;
> @@ -269,6 +279,7 @@ int qxl_log_command(PCIQXLDevice *qxl, const char *ring, QXLCommandExt *ext)
>          qxl_log_cmd_cursor(qxl, data, ext->group_id);
>          break;
>      }
> +out:
>      fprintf(stderr, "\n");
>      return 0;
>  }
> --
> 2.38.1
>
>


--
Marc-André Lureau

Re: [RFC PATCH-for-7.2 2/4] hw/display/qxl: Document qxl_phys2virt()

[Marc-André Lureau]

On Fri, Nov 25, 2022 at 7:41 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/display/qxl.h | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index e74de9579d..78b3a6c9ba 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -147,6 +147,25 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
>  #define QXL_DEFAULT_REVISION (QXL_REVISION_STABLE_V12 + 1)
>
>  /* qxl.c */
> +/**
> + * qxl_phys2virt: Get a pointer within a PCI VRAM memory region.
> + *
> + * @qxl: QXL device
> + * @phys: physical offset of buffer within the VRAM
> + * @group_id: memory slot group
> + *
> + * Returns a host pointer to a buffer placed at offset @phys within the
> + * active slot @group_id of the PCI VGA RAM memory region associated with
> + * the @qxl device. If the slot is inactive, or the offset is out
> + * of the memory region, returns NULL.
> + *
> + * Use with care; by the time this function returns, the returned pointer is
> + * not protected by RCU anymore.  If the caller is not within an RCU critical
> + * section and does not hold the iothread lock, it must have other means of
> + * protecting the pointer, such as a reference to the region that includes
> + * the incoming ram_addr_t.
> + *
> + */
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL phys, int group_id);
>  void qxl_set_guest_bug(PCIQXLDevice *qxl, const char *msg, ...)
>      G_GNUC_PRINTF(2, 3);
> --
> 2.38.1
>
>


--
Marc-André Lureau

Re: [RFC PATCH-for-7.2 4/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144)

[Marc-André Lureau]

Hi

On Fri, Nov 25, 2022 at 9:35 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Return NULL if the requested buffer size does not fit
> within the slot memory region.
>
> Reported-by: Wenxu Yin (@awxylitol)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1336
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  hw/display/qxl.c | 11 ++++++++++-
>  hw/display/qxl.h |  2 +-
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
> index 231d733250..e5e162f82d 100644
> --- a/hw/display/qxl.c
> +++ b/hw/display/qxl.c
> @@ -1462,7 +1462,7 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
>  void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
>                      size_t size)
>  {
> -    uint64_t offset;
> +    uint64_t offset, ptr_end_offset;
>      uint32_t slot;
>      void *ptr;
>
> @@ -1474,6 +1474,15 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
>          if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset)) {
>              return NULL;
>          }
> +
> +        ptr_end_offset = qxl->guest_slots[slot].offset + offset + size;

This is unlikely subject to int overflow, but perhaps it's worth
considering using some int128 instead?

> +        if (ptr_end_offset > memory_region_size(qxl->guest_slots[slot].mr)) {

> +            qxl_set_guest_bug(qxl,
> +                              "slot %d offset %"PRIu64" size %zu: "
> +                              "overrun by %"PRIu64" bytes\n",
> +                              slot, offset, size, ptr_end_offset - offset);
> +            return NULL;
> +        }
>          ptr = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr);
>          ptr += qxl->guest_slots[slot].offset;
>          ptr += offset;
> diff --git a/hw/display/qxl.h b/hw/display/qxl.h
> index bf03138ab4..7894bd5134 100644
> --- a/hw/display/qxl.h
> +++ b/hw/display/qxl.h
> @@ -157,7 +157,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(PCIQXLDevice, PCI_QXL)
>   *
>   * Returns a host pointer to a buffer placed at offset @phys within the
>   * active slot @group_id of the PCI VGA RAM memory region associated with
> - * the @qxl device. If the slot is inactive, or the offset is out
> + * the @qxl device. If the slot is inactive, or the offset + size are out
>   * of the memory region, returns NULL.
>   *
>   * Use with care; by the time this function returns, the returned pointer is
> --
> 2.38.1
>
>


-- 
Marc-André Lureau

Re: [RFC PATCH-for-7.2 3/4] hw/display/qxl: Pass requested buffer size to qxl_phys2virt()

[Philippe Mathieu-Daudé]

On 28/11/22 09:22, Marc-André Lureau wrote:
> On Fri, Nov 25, 2022 at 9:35 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Currently qxl_phys2virt() doesn't check for buffer overrun.
>> In order to do so in the next commit, pass the buffer size
>> as argument.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> 
> 
>> ---
>> RFC: Please double-check qxl_render_update_area_unlocked()
>> ---
>>   hw/display/qxl-logger.c | 11 ++++++++---
>>   hw/display/qxl-render.c | 11 +++++++----
>>   hw/display/qxl.c        | 14 +++++++++-----
>>   hw/display/qxl.h        |  4 +++-
>>   4 files changed, 27 insertions(+), 13 deletions(-)


>> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
>> index ca217004bf..1b0a50c1aa 100644
>> --- a/hw/display/qxl-render.c
>> +++ b/hw/display/qxl-render.c
>> @@ -107,7 +107,8 @@ static void qxl_render_update_area_unlocked(PCIQXLDevice *qxl)
>>           qxl->guest_primary.resized = 0;
>>           qxl->guest_primary.data = qxl_phys2virt(qxl,
>>                                                   qxl->guest_primary.surface.mem,
>> -                                                MEMSLOT_GROUP_GUEST);
>> +                                                MEMSLOT_GROUP_GUEST,
>> +                                                sizeof(uint32_t) * width * height);
> 
> It looks wrong, I think it should be:
> 
> qxl->guest_primary.abs_stride * height * qxl->guest_primary.bytes_pp

Isn't "bytes_pp" included in "abs_stride"?

If so, then "qxl->guest_primary.abs_stride * height" is enough..

Re: [RFC PATCH-for-7.2 4/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144)

[Philippe Mathieu-Daudé]

On 28/11/22 09:35, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Nov 25, 2022 at 9:35 PM Philippe Mathieu-Daudé
> <philmd@linaro.org> wrote:
>>
>> Return NULL if the requested buffer size does not fit
>> within the slot memory region.
>>
>> Reported-by: Wenxu Yin (@awxylitol)
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1336
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/display/qxl.c | 11 ++++++++++-
>>   hw/display/qxl.h |  2 +-
>>   2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/display/qxl.c b/hw/display/qxl.c
>> index 231d733250..e5e162f82d 100644
>> --- a/hw/display/qxl.c
>> +++ b/hw/display/qxl.c
>> @@ -1462,7 +1462,7 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl,
>>   void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
>>                       size_t size)
>>   {
>> -    uint64_t offset;
>> +    uint64_t offset, ptr_end_offset;
>>       uint32_t slot;
>>       void *ptr;
>>
>> @@ -1474,6 +1474,15 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, int group_id,
>>           if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset)) {
>>               return NULL;
>>           }
>> +
>> +        ptr_end_offset = qxl->guest_slots[slot].offset + offset + size;
> 
> This is unlikely subject to int overflow, but perhaps it's worth
> considering using some int128 instead?

If so this should be done after an audit of the codebase, many places 
are similar. I'll try to avoid using subtraction on "available size".


Note qxl_dirty_one_surface() seems to have the same issue when calling
qxl_set_dirty().

Should this check be moved to qxl_get_check_slot_offset()?

>> +        if (ptr_end_offset > memory_region_size(qxl->guest_slots[slot].mr)) {
> 
>> +            qxl_set_guest_bug(qxl,
>> +                              "slot %d offset %"PRIu64" size %zu: "
>> +                              "overrun by %"PRIu64" bytes\n",
>> +                              slot, offset, size, ptr_end_offset - offset);
>> +            return NULL;
>> +        }
>>           ptr = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr);
>>           ptr += qxl->guest_slots[slot].offset;
>>           ptr += offset;