Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Bill Spitzak]

On Mon, Sep 21, 2015 at 10:07 PM, Søren Sandmann 
wrote:

> Sure. The extra width check can't harm.
>

Actually it can, because it implies that such values *can* arrive at this
function, leading programmers to add tests to the calling functions, thus
leading to a large number of tests for conditions that cannot happen.

I would prefer to see an assert(width > 0) there.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Søren Sandmann]

Sure. The extra width check can't harm.


Søren



On Mon, Sep 21, 2015 at 10:10 PM, Siarhei Siamashka <
siarhei.siamas...@gmail.com> wrote:

> On Mon, 21 Sep 2015 16:43:46 -0400
> Søren Sandmann  wrote:
>
> > Regardless of who ends up listed as the patch author, this is
> >
> > Reviewed-by: soren.sandm...@gmail.com
> >
> > Søren
>
> Thanks! Is your Reviewed-by still valid after adding an extra
> "width <= 0" check to the patch?
>
> Best regards,
> Siarhei Siamashka
>
> > On Sep 21, 2015 3:07 PM, "Siarhei Siamashka" <
> siarhei.siamas...@gmail.com>
> > wrote:
> >
> > > On Mon, 21 Sep 2015 17:10:36 +0200
> > > l...@gnu.org (Ludovic Courtès) wrote:
> > >
> > > > Hello,
> > > >
> > > > The patch below intends to fix an arithmetic overflow occurring in a
> > > > pointer arithmetic context in ‘general_composite_rect’, as explained
> at:
> > > >
> > > >   https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6
> > >
> > > Sorry, I forgot to mention
> > >
> http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46
> > >
> > > We would also need a commit message for the patch. So it normally
> > > should be created with "git format-patch" command and sent to the
> > > mailing list using "git send-email".
> > >
> > > > The bug can most likely lead to a crash.
> > >
> > > Yes, I can confirm that the bug is reproducible on my x86-64 system:
> > >
> > > export CFLAGS="-O2 -m32" && ./autogen.sh
> > > ./configure --disable-libpng --disable-gtk && make
> > > setarch i686 -R test/stress-test
> > >
> > > > In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
> > > > insufficient to take 16-byte alignment constraints into account.
> > > > Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
> > > > Bpp == 4.
> > > >
> > > > Siarhei also suggests that more rewriting in needed in that part of
> the
> > > > code, but I’ll leave that to you.  ;-)
> > >
> > > Basically, I would probably do it in the following way:
> > >
> > >
> > > diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
> > > index 7cdea29..5ffa063 100644
> > > --- a/pixman/pixman-general.c
> > > +++ b/pixman/pixman-general.c
> > > @@ -155,23 +155,21 @@ general_composite_rect  (pixman_implementation_t
> > > *imp, #define
> > > ALIGN(addr) \
> > > ((uint8_t *)uintptr_t)(addr)) + 15) & (~15)))
> > > -src_buffer = ALIGN (scanline_buffer);
> > > -mask_buffer = ALIGN (src_buffer + width * Bpp);
> > > -dest_buffer = ALIGN (mask_buffer + width * Bpp);
> > > +if (_pixman_multiply_overflows_int (width, Bpp * 3))
> > > +   return;
> > >
> > > -if (ALIGN (dest_buffer + width * Bpp) >
> > > -   scanline_buffer + sizeof (stack_scanline_buffer))
> > > +if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3)
> > >  {
> > > scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32
> > > * 3);
> > > if (!scanline_buffer)
> > > return;
> > > -
> > > -   src_buffer = ALIGN (scanline_buffer);
> > > -   mask_buffer = ALIGN (src_buffer + width * Bpp);
> > > -   dest_buffer = ALIGN (mask_buffer + width * Bpp);
> > >  }
> > >
> > > +src_buffer = ALIGN (scanline_buffer);
> > > +mask_buffer = ALIGN (src_buffer + width * Bpp);
> > > +dest_buffer = ALIGN (mask_buffer + width * Bpp);
> > > +
> > >  if (width_flag == ITER_WIDE)
> > >  {
> > > /* To make sure there aren't any NANs in the buffers */
> > >
> > >
> > >
> > > This bug is your find and you should get credit for it :-)
> > > Please let me know if you:
> > > 1. are going to send an updated patch yourself.
> > > 2. want me to do this on your behalf (listing you as the patch author).
> > > 3. want me to submit a patch myself (listing you as the bug reporter).
> > >
> > >
> > > Also this is an important bugfix for a non-obvious problem, which can
> > > be really a PITA to debug. I would nominate it for a pixman-0.32.8
> > > bugfix release.
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Siarhei Siamashka]

On Mon, 21 Sep 2015 21:34:51 +0200
l...@gnu.org (Ludovic Courtès) wrote:

> Siarhei Siamashka  skribis:
> 
> > Sorry, I forgot to mention
> > http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46
> >
> > We would also need a commit message for the patch. So it normally
> > should be created with "git format-patch" command and sent to the
> > mailing list using "git send-email".
> 
> Right, sorry.  In fact I intended this message to be a RFC more than
> anything else.

OK, no problems.
 
> > Basically, I would probably do it in the following way:
> 
> Looks better to me, indeed.

Thanks.
 
> > This bug is your find and you should get credit for it :-)
> > Please let me know if you:
> > 1. are going to send an updated patch yourself.
> > 2. want me to do this on your behalf (listing you as the patch author).
> > 3. want me to submit a patch myself (listing you as the bug reporter).
> 
> I’m happy with #3 or #2 (the former would probably be more fair.)

OK, it's #3 then. Just submitted the proposed fix as
http://patchwork.freedesktop.org/patch/60052/

> > Also this is an important bugfix for a non-obvious problem, which can
> > be really a PITA to debug. I would nominate it for a pixman-0.32.8
> > bugfix release.
> 
> Yes, it’s probably a good idea.
> 
> It would be interesting to see whether/how the bug could be exploited in
> other ways.  For instance with, say, width = -20 % 2^32, one could
> arrange to overwrite the return address on the stack.

We are not supposed to have the invalid negative 'width' parameter
reaching so deep into the pixman code. Otherwise we are up to doing a
lot of checks for it all over the place. The negative width should be
handled much earlier:

http://cgit.freedesktop.org/pixman/tree/pixman/pixman.c?id=pixman-0.33.2#n241
Yes, a malicious user can craft artificial 'dest_x' and 'width'
parameters in such a way that 'dest_x + width' overflows and passes
this particular "check for empty operation" test. This probably
means that we should improve the "_pixman_compute_composite_region32()"
function too.

On the other hand, it is debatable how much of the input parameters
validation should be done on the pixman side (it is not free in terms
of CPU cycles) and how much of it can be delegated to the caller.
There are also special lightweight 'pixman_blt()' and 'pixman_fill()'
functions, which do no parameters validation at all.

Nevertheless, I have added an extra "width <= 0" check to the patch
because a bit of healthy paranoia is probably justified when dealing
with the buffers allocated on stack.

-- 
Best regards,
Siarhei Siamashka
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Siarhei Siamashka]

On Mon, 21 Sep 2015 16:43:46 -0400
Søren Sandmann  wrote:

> Regardless of who ends up listed as the patch author, this is
> 
> Reviewed-by: soren.sandm...@gmail.com
> 
> Søren

Thanks! Is your Reviewed-by still valid after adding an extra
"width <= 0" check to the patch?

Best regards,
Siarhei Siamashka

> On Sep 21, 2015 3:07 PM, "Siarhei Siamashka" 
> wrote:
> 
> > On Mon, 21 Sep 2015 17:10:36 +0200
> > l...@gnu.org (Ludovic Courtès) wrote:
> >
> > > Hello,
> > >
> > > The patch below intends to fix an arithmetic overflow occurring in a
> > > pointer arithmetic context in ‘general_composite_rect’, as explained at:
> > >
> > >   https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6
> >
> > Sorry, I forgot to mention
> > http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46
> >
> > We would also need a commit message for the patch. So it normally
> > should be created with "git format-patch" command and sent to the
> > mailing list using "git send-email".
> >
> > > The bug can most likely lead to a crash.
> >
> > Yes, I can confirm that the bug is reproducible on my x86-64 system:
> >
> > export CFLAGS="-O2 -m32" && ./autogen.sh
> > ./configure --disable-libpng --disable-gtk && make
> > setarch i686 -R test/stress-test
> >
> > > In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
> > > insufficient to take 16-byte alignment constraints into account.
> > > Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
> > > Bpp == 4.
> > >
> > > Siarhei also suggests that more rewriting in needed in that part of the
> > > code, but I’ll leave that to you.  ;-)
> >
> > Basically, I would probably do it in the following way:
> >
> >
> > diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
> > index 7cdea29..5ffa063 100644
> > --- a/pixman/pixman-general.c
> > +++ b/pixman/pixman-general.c
> > @@ -155,23 +155,21 @@ general_composite_rect  (pixman_implementation_t
> > *imp, #define
> > ALIGN(addr) \
> > ((uint8_t *)uintptr_t)(addr)) + 15) & (~15)))
> > -src_buffer = ALIGN (scanline_buffer);
> > -mask_buffer = ALIGN (src_buffer + width * Bpp);
> > -dest_buffer = ALIGN (mask_buffer + width * Bpp);
> > +if (_pixman_multiply_overflows_int (width, Bpp * 3))
> > +   return;
> >
> > -if (ALIGN (dest_buffer + width * Bpp) >
> > -   scanline_buffer + sizeof (stack_scanline_buffer))
> > +if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3)
> >  {
> > scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32
> > * 3);
> > if (!scanline_buffer)
> > return;
> > -
> > -   src_buffer = ALIGN (scanline_buffer);
> > -   mask_buffer = ALIGN (src_buffer + width * Bpp);
> > -   dest_buffer = ALIGN (mask_buffer + width * Bpp);
> >  }
> >
> > +src_buffer = ALIGN (scanline_buffer);
> > +mask_buffer = ALIGN (src_buffer + width * Bpp);
> > +dest_buffer = ALIGN (mask_buffer + width * Bpp);
> > +
> >  if (width_flag == ITER_WIDE)
> >  {
> > /* To make sure there aren't any NANs in the buffers */
> >
> >
> >
> > This bug is your find and you should get credit for it :-)
> > Please let me know if you:
> > 1. are going to send an updated patch yourself.
> > 2. want me to do this on your behalf (listing you as the patch author).
> > 3. want me to submit a patch myself (listing you as the bug reporter).
> >
> >
> > Also this is an important bugfix for a non-obvious problem, which can
> > be really a PITA to debug. I would nominate it for a pixman-0.32.8
> > bugfix release.
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

[Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Ludovic Courtès]

Hello,

The patch below intends to fix an arithmetic overflow occurring in a
pointer arithmetic context in ‘general_composite_rect’, as explained at:

  https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6

The bug can most likely lead to a crash.

In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
insufficient to take 16-byte alignment constraints into account.
Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
Bpp == 4.

Siarhei also suggests that more rewriting in needed in that part of the
code, but I’ll leave that to you.  ;-)

Thanks,
Ludo’.

Fix  whereby
an arithemitic overflow could occur while doing pointer arithmetic,
leading pixman to use an invalid address as the destination buffer.

--- pixman-0.32.6/pixman/pixman-general.c	2015-09-21 15:14:34.695981325 +0200
+++ pixman-0.32.6/pixman/pixman-general.c	2015-09-21 15:19:48.898355548 +0200
@@ -144,8 +144,7 @@ general_composite_rect  (pixman_implemen
 mask_buffer = ALIGN (src_buffer + width * Bpp);
 dest_buffer = ALIGN (mask_buffer + width * Bpp);
 
-if (ALIGN (dest_buffer + width * Bpp) >
-	scanline_buffer + sizeof (stack_scanline_buffer))
+if ((width + 1) * Bpp * 3 > sizeof (stack_scanline_buffer))
 {
 	scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32 * 3);
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Søren Sandmann]

Regardless of who ends up listed as the patch author, this is

Reviewed-by: soren.sandm...@gmail.com

Søren
On Sep 21, 2015 3:07 PM, "Siarhei Siamashka" 
wrote:

> On Mon, 21 Sep 2015 17:10:36 +0200
> l...@gnu.org (Ludovic Courtès) wrote:
>
> > Hello,
> >
> > The patch below intends to fix an arithmetic overflow occurring in a
> > pointer arithmetic context in ‘general_composite_rect’, as explained at:
> >
> >   https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6
>
> Sorry, I forgot to mention
> http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46
>
> We would also need a commit message for the patch. So it normally
> should be created with "git format-patch" command and sent to the
> mailing list using "git send-email".
>
> > The bug can most likely lead to a crash.
>
> Yes, I can confirm that the bug is reproducible on my x86-64 system:
>
> export CFLAGS="-O2 -m32" && ./autogen.sh
> ./configure --disable-libpng --disable-gtk && make
> setarch i686 -R test/stress-test
>
> > In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
> > insufficient to take 16-byte alignment constraints into account.
> > Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
> > Bpp == 4.
> >
> > Siarhei also suggests that more rewriting in needed in that part of the
> > code, but I’ll leave that to you.  ;-)
>
> Basically, I would probably do it in the following way:
>
>
> diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
> index 7cdea29..5ffa063 100644
> --- a/pixman/pixman-general.c
> +++ b/pixman/pixman-general.c
> @@ -155,23 +155,21 @@ general_composite_rect  (pixman_implementation_t
> *imp, #define
> ALIGN(addr) \
> ((uint8_t *)uintptr_t)(addr)) + 15) & (~15)))
> -src_buffer = ALIGN (scanline_buffer);
> -mask_buffer = ALIGN (src_buffer + width * Bpp);
> -dest_buffer = ALIGN (mask_buffer + width * Bpp);
> +if (_pixman_multiply_overflows_int (width, Bpp * 3))
> +   return;
>
> -if (ALIGN (dest_buffer + width * Bpp) >
> -   scanline_buffer + sizeof (stack_scanline_buffer))
> +if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3)
>  {
> scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32
> * 3);
> if (!scanline_buffer)
> return;
> -
> -   src_buffer = ALIGN (scanline_buffer);
> -   mask_buffer = ALIGN (src_buffer + width * Bpp);
> -   dest_buffer = ALIGN (mask_buffer + width * Bpp);
>  }
>
> +src_buffer = ALIGN (scanline_buffer);
> +mask_buffer = ALIGN (src_buffer + width * Bpp);
> +dest_buffer = ALIGN (mask_buffer + width * Bpp);
> +
>  if (width_flag == ITER_WIDE)
>  {
> /* To make sure there aren't any NANs in the buffers */
>
>
>
> This bug is your find and you should get credit for it :-)
> Please let me know if you:
> 1. are going to send an updated patch yourself.
> 2. want me to do this on your behalf (listing you as the patch author).
> 3. want me to submit a patch myself (listing you as the bug reporter).
>
>
> Also this is an important bugfix for a non-obvious problem, which can
> be really a PITA to debug. I would nominate it for a pixman-0.32.8
> bugfix release.
>
> --
> Best regards,
> Siarhei Siamashka
> ___
> Pixman mailing list
> Pixman@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/pixman
>
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman

Re: [Pixman] [PATCH] Fix arithmetic overflow in pointer arithmetic in ‘general_composite_rect’

[Siarhei Siamashka]

On Mon, 21 Sep 2015 17:10:36 +0200
l...@gnu.org (Ludovic Courtès) wrote:

> Hello,
> 
> The patch below intends to fix an arithmetic overflow occurring in a
> pointer arithmetic context in ‘general_composite_rect’, as explained at:
> 
>   https://bugs.freedesktop.org/show_bug.cgi?id=92027#c6

Sorry, I forgot to mention
http://cgit.freedesktop.org/pixman/tree/README?id=pixman-0.33.2#n46

We would also need a commit message for the patch. So it normally
should be created with "git format-patch" command and sent to the
mailing list using "git send-email".

> The bug can most likely lead to a crash.

Yes, I can confirm that the bug is reproducible on my x86-64 system:

export CFLAGS="-O2 -m32" && ./autogen.sh
./configure --disable-libpng --disable-gtk && make
setarch i686 -R test/stress-test

> In a preliminary review, Siarhei Siamashka notes that ‘width + 1’ is
> insufficient to take 16-byte alignment constraints into account.
> Indeed, AFAICS, it is sufficient when Bpp == 16 but probably not when
> Bpp == 4.
>
> Siarhei also suggests that more rewriting in needed in that part of the
> code, but I’ll leave that to you.  ;-)

Basically, I would probably do it in the following way:


diff --git a/pixman/pixman-general.c b/pixman/pixman-general.c
index 7cdea29..5ffa063 100644
--- a/pixman/pixman-general.c
+++ b/pixman/pixman-general.c
@@ -155,23 +155,21 @@ general_composite_rect  (pixman_implementation_t
*imp, #define
ALIGN(addr) \
((uint8_t *)uintptr_t)(addr)) + 15) & (~15))) 
-src_buffer = ALIGN (scanline_buffer);
-mask_buffer = ALIGN (src_buffer + width * Bpp);
-dest_buffer = ALIGN (mask_buffer + width * Bpp);
+if (_pixman_multiply_overflows_int (width, Bpp * 3))
+   return;
 
-if (ALIGN (dest_buffer + width * Bpp) >
-   scanline_buffer + sizeof (stack_scanline_buffer))
+if (width * Bpp * 3 > sizeof (stack_scanline_buffer) - 32 * 3)
 {
scanline_buffer = pixman_malloc_ab_plus_c (width, Bpp * 3, 32
* 3); 
if (!scanline_buffer)
return;
-
-   src_buffer = ALIGN (scanline_buffer);
-   mask_buffer = ALIGN (src_buffer + width * Bpp);
-   dest_buffer = ALIGN (mask_buffer + width * Bpp);
 }
 
+src_buffer = ALIGN (scanline_buffer);
+mask_buffer = ALIGN (src_buffer + width * Bpp);
+dest_buffer = ALIGN (mask_buffer + width * Bpp);
+
 if (width_flag == ITER_WIDE)
 {
/* To make sure there aren't any NANs in the buffers */



This bug is your find and you should get credit for it :-)
Please let me know if you:
1. are going to send an updated patch yourself.
2. want me to do this on your behalf (listing you as the patch author).
3. want me to submit a patch myself (listing you as the bug reporter).


Also this is an important bugfix for a non-obvious problem, which can
be really a PITA to debug. I would nominate it for a pixman-0.32.8
bugfix release.

-- 
Best regards,
Siarhei Siamashka
___
Pixman mailing list
Pixman@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/pixman