hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

[PATCH] hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

Posted by Michael Tokarev 11 months, 3 weeks ago

In case of multiple chunks, code in qxl_unpack_chunks() takes size of the
wrong (next in the chain) chunk, instead of using current chunk size.
This leads to wrong number of bytes being copied, and to crashes if next
chunk size is larger than the current one.

Based on the code by Gao Yong.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1628
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 hw/display/qxl-render.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
index eda6d3de37..c6a9ac1da1 100644
--- a/hw/display/qxl-render.c
+++ b/hw/display/qxl-render.c
@@ -222,6 +222,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
     uint32_t max_chunks = 32;
     size_t offset = 0;
     size_t bytes;
+    QXLPHYSICAL next_chunk_phys = 0;
 
     for (;;) {
         bytes = MIN(size - offset, chunk->data_size);
@@ -230,7 +231,15 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
         if (offset == size) {
             return;
         }
-        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
+        next_chunk_phys = chunk->next_chunk;
+        /* fist time, only get the next chunk's data size */
+        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
+                              sizeof(QXLDataChunk));
+        if (!chunk) {
+            return;
+        }
+        /* second time, check data size and get data */
+        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
                               sizeof(QXLDataChunk) + chunk->data_size);
         if (!chunk) {
             return;
-- 
2.39.5

Re: [PATCH] hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

Posted by Philippe Mathieu-Daudé 6 months, 2 weeks ago

On 21/2/25 14:48, Michael Tokarev wrote:
> In case of multiple chunks, code in qxl_unpack_chunks() takes size of the
> wrong (next in the chain) chunk, instead of using current chunk size.
> This leads to wrong number of bytes being copied, and to crashes if next
> chunk size is larger than the current one.
> 
> Based on the code by Gao Yong.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1628
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   hw/display/qxl-render.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index eda6d3de37..c6a9ac1da1 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -222,6 +222,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>       uint32_t max_chunks = 32;
>       size_t offset = 0;
>       size_t bytes;
> +    QXLPHYSICAL next_chunk_phys = 0;

Thanks, queued (without zero-initialization).

>   
>       for (;;) {
>           bytes = MIN(size - offset, chunk->data_size);
> @@ -230,7 +231,15 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>           if (offset == size) {
>               return;
>           }
> -        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
> +        next_chunk_phys = chunk->next_chunk;
> +        /* fist time, only get the next chunk's data size */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
> +                              sizeof(QXLDataChunk));
> +        if (!chunk) {
> +            return;
> +        }
> +        /* second time, check data size and get data */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
>                                 sizeof(QXLDataChunk) + chunk->data_size);
>           if (!chunk) {
>               return;

Re: [PATCH] hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

Posted by Michael Tokarev 6 months, 2 weeks ago

On 28.07.2025 14:06, Philippe Mathieu-Daudé wrote:
> On 21/2/25 14:48, Michael Tokarev wrote:
>> In case of multiple chunks, code in qxl_unpack_chunks() takes size of the
>> wrong (next in the chain) chunk, instead of using current chunk size.
>> This leads to wrong number of bytes being copied, and to crashes if next
>> chunk size is larger than the current one.
>>
>> Based on the code by Gao Yong.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1628
>> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
>> ---
>>   hw/display/qxl-render.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
>> index eda6d3de37..c6a9ac1da1 100644
>> --- a/hw/display/qxl-render.c
>> +++ b/hw/display/qxl-render.c
>> @@ -222,6 +222,7 @@ static void qxl_unpack_chunks(void *dest, size_t 
>> size, PCIQXLDevice *qxl,
>>       uint32_t max_chunks = 32;
>>       size_t offset = 0;
>>       size_t bytes;
>> +    QXLPHYSICAL next_chunk_phys = 0;
> 
> Thanks, queued (without zero-initialization).

Heh. Indeed, the init isn't needed here.
But you're a bit too late: this version has already been
applied.  Dunno if it's worth the effort to remove the
initializer here, - probably not.

Philippe, you can at least pick up another patch from the
trivial patches queue (since I don't have any other patches) -
this is "roms/Makefile: fix npcmNxx_bootrom build rules".
(There are 2 more changes pending in this area though -
it is the rules for ast270x0 vbootrom, waiting for the
upstream git tree to fix a build bug in there).

Thanks,

/mjt

Re: [PATCH] hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

Posted by Thomas Huth 6 months, 2 weeks ago

On 21/02/2025 14.48, Michael Tokarev wrote:
> In case of multiple chunks, code in qxl_unpack_chunks() takes size of the
> wrong (next in the chain) chunk, instead of using current chunk size.
> This leads to wrong number of bytes being copied, and to crashes if next
> chunk size is larger than the current one.
> 
> Based on the code by Gao Yong.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1628
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   hw/display/qxl-render.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index eda6d3de37..c6a9ac1da1 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -222,6 +222,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>       uint32_t max_chunks = 32;
>       size_t offset = 0;
>       size_t bytes;
> +    QXLPHYSICAL next_chunk_phys = 0;
>   
>       for (;;) {
>           bytes = MIN(size - offset, chunk->data_size);
> @@ -230,7 +231,15 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>           if (offset == size) {
>               return;
>           }
> -        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
> +        next_chunk_phys = chunk->next_chunk;
> +        /* fist time, only get the next chunk's data size */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
> +                              sizeof(QXLDataChunk));
> +        if (!chunk) {
> +            return;
> +        }
> +        /* second time, check data size and get data */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
>                                 sizeof(QXLDataChunk) + chunk->data_size);

Looks reasonable to me, and I think it's also simple enough for qemu-trivial 
(now on CC:).

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] hw/display/qxl-render.c: fix qxl_unpack_chunks() chunk size calculation

Posted by Michael Tokarev 11 months, 1 week ago

A friendly ping?
This one probably should go to qemu-stable too.

Thanks,

/mjt

21.02.2025 16:48, Michael Tokarev wrote:
> In case of multiple chunks, code in qxl_unpack_chunks() takes size of the
> wrong (next in the chain) chunk, instead of using current chunk size.
> This leads to wrong number of bytes being copied, and to crashes if next
> chunk size is larger than the current one.
> 
> Based on the code by Gao Yong.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1628
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   hw/display/qxl-render.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/display/qxl-render.c b/hw/display/qxl-render.c
> index eda6d3de37..c6a9ac1da1 100644
> --- a/hw/display/qxl-render.c
> +++ b/hw/display/qxl-render.c
> @@ -222,6 +222,7 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>       uint32_t max_chunks = 32;
>       size_t offset = 0;
>       size_t bytes;
> +    QXLPHYSICAL next_chunk_phys = 0;
>   
>       for (;;) {
>           bytes = MIN(size - offset, chunk->data_size);
> @@ -230,7 +231,15 @@ static void qxl_unpack_chunks(void *dest, size_t size, PCIQXLDevice *qxl,
>           if (offset == size) {
>               return;
>           }
> -        chunk = qxl_phys2virt(qxl, chunk->next_chunk, group_id,
> +        next_chunk_phys = chunk->next_chunk;
> +        /* fist time, only get the next chunk's data size */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
> +                              sizeof(QXLDataChunk));
> +        if (!chunk) {
> +            return;
> +        }
> +        /* second time, check data size and get data */
> +        chunk = qxl_phys2virt(qxl, next_chunk_phys, group_id,
>                                 sizeof(QXLDataChunk) + chunk->data_size);
>           if (!chunk) {
>               return;