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

[Siarhei Siamashka]

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

> Siarhei Siamashka <siarhei.siamashka at gmail.com> 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