| Message ID | 20221125173119.46665-2-philmd@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/display/qxl: Avoid buffer overrun in qxl_phys2virt() | expand |
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 > >
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;
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
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(-)