On Mon, 28 Nov 2022 at 08:53, Philippe Mathieu-Daudé <phi...@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é <phi...@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. > + size_available -= qxl->guest_slots[slot].offset + offset; > + if (size_requested > size_available) { > + qxl_set_guest_bug(qxl, > + "slot %d offset %"PRIu64" size %zu: " > + "overrun by %"PRIu64" bytes\n", > + slot, offset, size_requested, > + size_requested - size_available); > + return false; > + } > + > *s = slot; > *o = offset; > return true; > @@ -1471,7 +1485,7 @@ void *qxl_phys2virt(PCIQXLDevice *qxl, QXLPHYSICAL > pqxl, int group_id, > offset = le64_to_cpu(pqxl) & 0xffffffffffff; > return (void *)(intptr_t)offset; > case MEMSLOT_GROUP_GUEST: > - if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset)) { > + if (!qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size)) { > return NULL; > } > ptr = memory_region_get_ram_ptr(qxl->guest_slots[slot].mr); > @@ -1937,9 +1951,9 @@ static void qxl_dirty_one_surface(PCIQXLDevice *qxl, > QXLPHYSICAL pqxl, > uint32_t slot; > bool rc; > > - rc = qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset); > - assert(rc == true); > size = (uint64_t)height * abs(stride); > + rc = qxl_get_check_slot_offset(qxl, pqxl, &slot, &offset, size); > + assert(rc == true); > trace_qxl_surfaces_dirty(qxl->id, offset, size); > qxl_set_dirty(qxl->guest_slots[slot].mr, > qxl->guest_slots[slot].offset + 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 > >