| Message ID | 20231121093840.2121195-1-manos.pitsidianakis@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-8.2] ui/pixman-minimal.h: fix empty allocation | expand |
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 >
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.
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;
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(-) base-commit: af9264da80073435fd78944bc5a46e695897d7e5