|
From: | Philippe Mathieu-Daudé |
Subject: | Re: [RFC PATCH-for-7.2 4/5] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144) |
Date: | Mon, 28 Nov 2022 16:46:29 +0100 |
User-agent: | Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.5.0 |
On 28/11/22 16:32, Stefan Hajnoczi wrote:
On Mon, 28 Nov 2022 at 10:25, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:On 28/11/22 16:16, Stefan Hajnoczi wrote:On Mon, 28 Nov 2022 at 08:53, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:Have qxl_get_check_slot_offset() return false if the requested buffer size does not fit within the slot memory region. Similarly qxl_phys2virt() now returns NULL in such case, and qxl_dirty_one_surface() aborts. This avoids buffer overrun in the host pointer returned by memory_region_get_ram_ptr(). Fixes: CVE-2022-4144 (out-of-bounds read) 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 | 22 ++++++++++++++++++---- hw/display/qxl.h | 2 +- 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/hw/display/qxl.c b/hw/display/qxl.c index 231d733250..afa157d327 100644 --- a/hw/display/qxl.c +++ b/hw/display/qxl.c @@ -1424,11 +1424,13 @@ static void qxl_reset_surfaces(PCIQXLDevice *d) /* can be also called from spice server thread context */ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, - uint32_t *s, uint64_t *o) + uint32_t *s, uint64_t *o, + size_t size_requested) { uint64_t phys = le64_to_cpu(pqxl); uint32_t slot = (phys >> (64 - 8)) & 0xff; uint64_t offset = phys & 0xffffffffffff; + uint64_t size_available; if (slot >= NUM_MEMSLOTS) { qxl_set_guest_bug(qxl, "slot too large %d >= %d", slot, @@ -1453,6 +1455,18 @@ static bool qxl_get_check_slot_offset(PCIQXLDevice *qxl, QXLPHYSICAL pqxl, return false; } + size_available = memory_region_size(qxl->guest_slots[slot].mr); + assert(qxl->guest_slots[slot].offset + offset < size_available);Can this assertion be triggered by the guest (via an invalid pqxl value)? I think the answer is no, but I don't know the the qxl code well enough to be sure.'qxl->guest_slots[slot].offset' is initialized in qxl_add_memslot() (host); 'size_available' also comes from the host, but 'offset' comes from the guest via 'QXLPHYSICAL pqxl' IIUC. I added this check to avoid overflow, but it can be changed to return an error.Yes, please.
Or I could use Int128 to do arithmetic, but various other places do it this way without checking overflow with memory_region_size(). Such API change should be global and is out of the scope of this CVE fix IMO.
Aside from concerns about -DNDEBUG, which builds without assertions,
This isn't an issue anymore since 262a69f428 ("osdep.h: Prohibit disabling assert() in supported builds").
there is also a DoS issue with nested virt where an L2 guest shouldn't be able to abort the L1 guest's QEMU by triggering an assertion in a pass through device. Guest input validation should use explicit error checking code instead of assert(3).
Certainly.
[Prev in Thread] | Current Thread | [Next in Thread] |