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é
<phi...@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é <phi...@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;