In the minimal pixman API stub that is used when the real pixman
dependency is missing a NULL dereference happens when
virtio-gpu-rutabaga allocates a pixman image with bits = NULL and
rowstride_bytes = zero. A buffer of rowstride_bytes * height is
allocated which is NULL. However, in that scenario pixman calculates a
new stride value based on given width, height and format size.

This commit adds a helper function that performs the same logic as
pixman.

Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
---
 include/ui/pixman-minimal.h | 48 +++++++++++++++++++++++++++++++++++--
 1 file changed, 46 insertions(+), 2 deletions(-)

diff --git a/include/ui/pixman-minimal.h b/include/ui/pixman-minimal.h
index efcf570c9e..6dd7de1c7e 100644
--- a/include/ui/pixman-minimal.h
+++ b/include/ui/pixman-minimal.h
@@ -113,6 +113,45 @@ typedef struct pixman_color {
     uint16_t    alpha;
 } pixman_color_t;
 
+static inline uint32_t *create_bits(pixman_format_code_t format,
+                                    int width,
+                                    int height,
+                                    int *rowstride_bytes)
+{
+    int stride = 0;
+    size_t buf_size = 0;
+    int bpp = PIXMAN_FORMAT_BPP(format);
+
+    /*
+     * Calculate the following while checking for overflow truncation:
+     * stride = ((width * bpp + 0x1f) >> 5) * sizeof(uint32_t);
+     */
+
+    if (unlikely(__builtin_mul_overflow(width, bpp, &stride))) {
+        return NULL;
+    }
+
+    if (unlikely(__builtin_add_overflow(stride, 0x1f, &stride))) {
+        return NULL;
+    }
+
+    stride >>= 5;
+
+    stride *= sizeof(uint32_t);
+
+    if (unlikely(__builtin_mul_overflow((size_t) height,
+                                        (size_t) stride,
+                                        &buf_size))) {
+        return NULL;
+    }
+
+    if (rowstride_bytes) {
+        *rowstride_bytes = stride;
+    }
+
+    return g_malloc0(buf_size);
+}
+
 static inline pixman_image_t *pixman_image_create_bits(pixman_format_code_t format,
                                                        int width,
                                                        int height,
@@ -123,13 +162,18 @@ static inline pixman_image_t *pixman_image_create_bits(pixman_format_code_t form
 
     i->width = width;
     i->height = height;
-    i->stride = rowstride_bytes ?: width * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
     i->format = format;
     if (bits) {
         i->data = bits;
     } else {
-        i->free_me = i->data = g_malloc0(rowstride_bytes * height);
+        i->free_me = i->data =
+            create_bits(format, width, height, &rowstride_bytes);
+        if (width && height) {
+            assert(i->data);
+        }
     }
+    i->stride = rowstride_bytes ? rowstride_bytes :
+                            width * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
     i->ref_count = 1;
 
     return i;

base-commit: af9264da80073435fd78944bc5a46e695897d7e5
-- 
2.39.2

21.11.2023 12:38, Manos Pitsidianakis :
> In the minimal pixman API stub that is used when the real pixman
> dependency is missing a NULL dereference happens when
> virtio-gpu-rutabaga allocates a pixman image with bits = NULL and
> rowstride_bytes = zero. A buffer of rowstride_bytes * height is
> allocated which is NULL. However, in that scenario pixman calculates a
> new stride value based on given width, height and format size.

So, does rutabaga depend on pixman (as mentioned on irc), or can the
minimal in-qemu version (with this fix) be used?

/mjt

On Thu, 23 Nov 2023 12:15, Michael Tokarev <mjt@tls.msk.ru> wrote:
>21.11.2023 12:38, Manos Pitsidianakis :
>> In the minimal pixman API stub that is used when the real pixman
>> dependency is missing a NULL dereference happens when
>> virtio-gpu-rutabaga allocates a pixman image with bits = NULL and
>> rowstride_bytes = zero. A buffer of rowstride_bytes * height is
>> allocated which is NULL. However, in that scenario pixman calculates a
>> new stride value based on given width, height and format size.
>
>So, does rutabaga depend on pixman (as mentioned on irc), or can the
>minimal in-qemu version (with this fix) be used?

I think it probably does not depend on missing functionality; it seems 
to only be passing the allocated data to the external library and not 
pixman types. I'd have to check on amd64 where I previously ran rutabaga 
successfuly in qemu; now I'm trying to get it to work on aarch64 and I'm 
trying to figure out some control message errors from the guest kernel 
driver. When/if I try it again on amd64 and it's not working, I will 
either report or send a patch.

Hi Manos

On Tue, Nov 21, 2023 at 1:38 PM Manos Pitsidianakis
<manos.pitsidianakis@linaro.org> wrote:
>
> In the minimal pixman API stub that is used when the real pixman
> dependency is missing a NULL dereference happens when
> virtio-gpu-rutabaga allocates a pixman image with bits = NULL and
> rowstride_bytes = zero. A buffer of rowstride_bytes * height is
> allocated which is NULL. However, in that scenario pixman calculates a
> new stride value based on given width, height and format size.
>
> This commit adds a helper function that performs the same logic as
> pixman.
>

Thanks a lot for investigating this and providing a solution!
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> Signed-off-by: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> ---
>  include/ui/pixman-minimal.h | 48 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
>
> diff --git a/include/ui/pixman-minimal.h b/include/ui/pixman-minimal.h
> index efcf570c9e..6dd7de1c7e 100644
> --- a/include/ui/pixman-minimal.h
> +++ b/include/ui/pixman-minimal.h
> @@ -113,6 +113,45 @@ typedef struct pixman_color {
>      uint16_t    alpha;
>  } pixman_color_t;
>
> +static inline uint32_t *create_bits(pixman_format_code_t format,
> +                                    int width,
> +                                    int height,
> +                                    int *rowstride_bytes)
> +{
> +    int stride = 0;
> +    size_t buf_size = 0;
> +    int bpp = PIXMAN_FORMAT_BPP(format);
> +
> +    /*
> +     * Calculate the following while checking for overflow truncation:
> +     * stride = ((width * bpp + 0x1f) >> 5) * sizeof(uint32_t);
> +     */
> +
> +    if (unlikely(__builtin_mul_overflow(width, bpp, &stride))) {
> +        return NULL;
> +    }
> +
> +    if (unlikely(__builtin_add_overflow(stride, 0x1f, &stride))) {
> +        return NULL;
> +    }
> +
> +    stride >>= 5;
> +
> +    stride *= sizeof(uint32_t);
> +
> +    if (unlikely(__builtin_mul_overflow((size_t) height,
> +                                        (size_t) stride,
> +                                        &buf_size))) {
> +        return NULL;
> +    }
> +
> +    if (rowstride_bytes) {
> +        *rowstride_bytes = stride;
> +    }
> +
> +    return g_malloc0(buf_size);
> +}
> +
>  static inline pixman_image_t *pixman_image_create_bits(pixman_format_code_t format,
>                                                         int width,
>                                                         int height,
> @@ -123,13 +162,18 @@ static inline pixman_image_t *pixman_image_create_bits(pixman_format_code_t form
>
>      i->width = width;
>      i->height = height;
> -    i->stride = rowstride_bytes ?: width * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
>      i->format = format;
>      if (bits) {
>          i->data = bits;
>      } else {
> -        i->free_me = i->data = g_malloc0(rowstride_bytes * height);
> +        i->free_me = i->data =
> +            create_bits(format, width, height, &rowstride_bytes);
> +        if (width && height) {
> +            assert(i->data);
> +        }
>      }
> +    i->stride = rowstride_bytes ? rowstride_bytes :
> +                            width * DIV_ROUND_UP(PIXMAN_FORMAT_BPP(format), 8);
>      i->ref_count = 1;
>
>      return i;
>
> base-commit: af9264da80073435fd78944bc5a46e695897d7e5
> --
> 2.39.2
>