image: prevent invalid ptr access for > 4GB images

Submitted by Adrian Johnson on March 9, 2017, 11:01 a.m.

Details

Message ID 4fb4e107-8872-37ed-b209-acd5be5b1e6c@redneon.com
State New
Series "image: prevent invalid ptr access for > 4GB images"
Headers show

Commit Message

Adrian Johnson March 9, 2017, 11:01 a.m.
I wrote this patch [1] for bug 98165 last year. I found it is also
needed to fix a poppler bug [2]. Any objections to pushing it?

[1] https://bugs.freedesktop.org/show_bug.cgi?id=98165#c6
[2] https://bugs.freedesktop.org/show_bug.cgi?id=100056

Patch hide | download patch | download mbox

From c812d1c1935cccf096a60ad904e640fdc83bd41c Mon Sep 17 00:00:00 2001
From: Adrian Johnson <ajohnson@redneon.com>
Date: Thu, 20 Oct 2016 21:12:30 +1030
Subject: [PATCH] image: prevent invalid ptr access for > 4GB images

Image data is often accessed using:

  image->data + y * image->stride

On 64-bit achitectures if the image data is > 4GB, this computation
will overflow since both y and stride are 32-bit types.

https://bugs.freedesktop.org/show_bug.cgi?id=98165
---
 boilerplate/cairo-boilerplate.c     | 4 +++-
 src/cairo-image-compositor.c        | 4 ++--
 src/cairo-image-surface-private.h   | 2 +-
 src/cairo-mesh-pattern-rasterizer.c | 2 +-
 src/cairo-png.c                     | 2 +-
 src/cairo-script-surface.c          | 3 ++-
 6 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/boilerplate/cairo-boilerplate.c b/boilerplate/cairo-boilerplate.c
index 7fdbf79..4804dea 100644
--- a/boilerplate/cairo-boilerplate.c
+++ b/boilerplate/cairo-boilerplate.c
@@ -42,6 +42,7 @@ 
 #undef CAIRO_VERSION_H
 #include "../cairo-version.h"
 
+#include <stddef.h>
 #include <stdlib.h>
 #include <ctype.h>
 #include <assert.h>
@@ -976,7 +977,8 @@  cairo_surface_t *
 cairo_boilerplate_image_surface_create_from_ppm_stream (FILE *file)
 {
     char format;
-    int width, height, stride;
+    int width, height;
+    ptrdiff_t stride;
     int x, y;
     unsigned char *data;
     cairo_surface_t *image = NULL;
diff --git a/src/cairo-image-compositor.c b/src/cairo-image-compositor.c
index 48072f8..3ca0006 100644
--- a/src/cairo-image-compositor.c
+++ b/src/cairo-image-compositor.c
@@ -1575,7 +1575,7 @@  typedef struct _cairo_image_span_renderer {
     pixman_image_t *src, *mask;
     union {
 	struct fill {
-	    int stride;
+	    ptrdiff_t stride;
 	    uint8_t *data;
 	    uint32_t pixel;
 	} fill;
@@ -1594,7 +1594,7 @@  typedef struct _cairo_image_span_renderer {
 	struct finish {
 	    cairo_rectangle_int_t extents;
 	    int src_x, src_y;
-	    int stride;
+	    ptrdiff_t stride;
 	    uint8_t *data;
 	} mask;
     } u;
diff --git a/src/cairo-image-surface-private.h b/src/cairo-image-surface-private.h
index 8ca694c..7e78d61 100644
--- a/src/cairo-image-surface-private.h
+++ b/src/cairo-image-surface-private.h
@@ -71,7 +71,7 @@  struct _cairo_image_surface {
 
     int width;
     int height;
-    int stride;
+    ptrdiff_t stride;
     int depth;
 
     unsigned owns_data : 1;
diff --git a/src/cairo-mesh-pattern-rasterizer.c b/src/cairo-mesh-pattern-rasterizer.c
index 1b63ca8..e7f0db6 100644
--- a/src/cairo-mesh-pattern-rasterizer.c
+++ b/src/cairo-mesh-pattern-rasterizer.c
@@ -470,7 +470,7 @@  draw_pixel (unsigned char *data, int width, int height, int stride,
 	tg += tg >> 16;
 	tb += tb >> 16;
 
-	*((uint32_t*) (data + y*stride + 4*x)) = ((ta << 16) & 0xff000000) |
+	*((uint32_t*) (data + y*(ptrdiff_t)stride + 4*x)) = ((ta << 16) & 0xff000000) |
 	    ((tr >> 8) & 0xff0000) | ((tg >> 16) & 0xff00) | (tb >> 24);
     }
 }
diff --git a/src/cairo-png.c b/src/cairo-png.c
index 562b743..aa8c227 100644
--- a/src/cairo-png.c
+++ b/src/cairo-png.c
@@ -673,7 +673,7 @@  read_png (struct png_read_closure_t *png_closure)
     }
 
     for (i = 0; i < png_height; i++)
-        row_pointers[i] = &data[i * stride];
+        row_pointers[i] = &data[i * (ptrdiff_t)stride];
 
     png_read_image (png, row_pointers);
     png_read_end (png, info);
diff --git a/src/cairo-script-surface.c b/src/cairo-script-surface.c
index ea0117d..91e4baa 100644
--- a/src/cairo-script-surface.c
+++ b/src/cairo-script-surface.c
@@ -1202,7 +1202,8 @@  static cairo_status_t
 _write_image_surface (cairo_output_stream_t *output,
 		      const cairo_image_surface_t *image)
 {
-    int stride, row, width;
+    int row, width;
+    ptrdiff_t stride;
     uint8_t row_stack[CAIRO_STACK_BUFFER_SIZE];
     uint8_t *rowdata;
     uint8_t *data;
-- 
2.1.4


Comments

Petr Kobalíček March 9, 2017, 4:23 p.m.
Since `cairo_format_stride_for_width()` has no knowledge about the width it
cannot return -1 in case that stride * height would overflow. I think this
must be checked.

On Thu, Mar 9, 2017 at 12:01 PM, Adrian Johnson <ajohnson@redneon.com>
wrote:

> I wrote this patch [1] for bug 98165 last year. I found it is also
> needed to fix a poppler bug [2]. Any objections to pushing it?
>
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98165#c6
> [2] https://bugs.freedesktop.org/show_bug.cgi?id=100056
>
>
> --
> cairo mailing list
> cairo@cairographics.org
> https://lists.cairographics.org/mailman/listinfo/cairo
>
Petr Kobalíček March 9, 2017, 4:23 p.m.
I meant `cairo_format_stride_for_width()` has no knowledge about the height.

On Thu, Mar 9, 2017 at 5:23 PM, Petr Kobalíček <kobalicek.petr@gmail.com>
wrote:

> Since `cairo_format_stride_for_width()` has no knowledge about the width
> it cannot return -1 in case that stride * height would overflow. I think
> this must be checked.
>
> On Thu, Mar 9, 2017 at 12:01 PM, Adrian Johnson <ajohnson@redneon.com>
> wrote:
>
>> I wrote this patch [1] for bug 98165 last year. I found it is also
>> needed to fix a poppler bug [2]. Any objections to pushing it?
>>
>> [1] https://bugs.freedesktop.org/show_bug.cgi?id=98165#c6
>> [2] https://bugs.freedesktop.org/show_bug.cgi?id=100056
>>
>>
>> --
>> cairo mailing list
>> cairo@cairographics.org
>> https://lists.cairographics.org/mailman/listinfo/cairo
>>
>
>
Adrian Johnson June 15, 2017, 12:05 p.m.
As the maximum image width is 32767 I don't see how this could overflow.

On 10/03/17 02:53, Petr Kobalíček wrote:
> Since `cairo_format_stride_for_width()` has no knowledge about the width
> it cannot return -1 in case that stride * height would overflow. I think
> this must be checked.
> 
> On Thu, Mar 9, 2017 at 12:01 PM, Adrian Johnson <ajohnson@redneon.com
> <mailto:ajohnson@redneon.com>> wrote:
> 
>     I wrote this patch [1] for bug 98165 last year. I found it is also
>     needed to fix a poppler bug [2]. Any objections to pushing it?
> 
>     [1] https://bugs.freedesktop.org/show_bug.cgi?id=98165#c6
>     <https://bugs.freedesktop.org/show_bug.cgi?id=98165#c6>
>     [2] https://bugs.freedesktop.org/show_bug.cgi?id=100056
>     <https://bugs.freedesktop.org/show_bug.cgi?id=100056>
> 
> 
>     --
>     cairo mailing list
>     cairo@cairographics.org <mailto:cairo@cairographics.org>
>     https://lists.cairographics.org/mailman/listinfo/cairo
>     <https://lists.cairographics.org/mailman/listinfo/cairo>
> 
> 
> 
>
Chris Wilson June 15, 2017, 12:26 p.m.
Quoting Adrian Johnson (2017-06-15 13:05:33)
> As the maximum image width is 32767 I don't see how this could overflow.

cairo_image_surface_create_for_data() lacks overflow protection against
stride. That is the only entry point that doesn't have a check afaik.
-Chris