diff mbox series

[RFC,PATCH-for-7.2,4/4] hw/display/qxl: Avoid buffer overrun in qxl_phys2virt (CVE-2022-4144)

Message ID 20221125173119.46665-2-philmd@linaro.org
State New
Headers show
Series hw/display/qxl: Avoid buffer overrun in qxl_phys2virt() | expand

Commit Message

Philippe Mathieu-Daudé Nov. 25, 2022, 5:31 p.m. UTC 
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(-)

Comments

Marc-André Lureau Nov. 28, 2022, 8:35 a.m. UTC | #1 
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
>
>
Philippe Mathieu-Daudé Nov. 28, 2022, 11:14 a.m. UTC | #2 
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 mbox series

Patch

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