| Message ID | 20221128134832.84867-5-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/display/qxl: Avoid buffer overrun in qxl_phys2virt() | expand |
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. > + 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 > >
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. Thanks, Phil.
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. Aside from concerns about -DNDEBUG, which builds without assertions, 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). Thanks, Stefan
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.
On Mon, 28 Nov 2022 at 10:46, Philippe Mathieu-Daudé <philmd@linaro.org> wrote: > > 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"). I didn't know about that. Thanks! Stefan
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); + 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
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(-)