typo in mod() macro leads to 3rd-party controllable Xorg crash/exploit

Description of problem:
When attempting to load http://www.buffalonews.com/entertainment/moviestv/index.html Xorg crashes:

#0 0x0020161f in fbCopyAreammx (pSrc=0x8cdea90, pDst=0x8cb9620, src_x=0, src_y=3846, dst_x=18, dst_y=3871, width=920, height=60788) at fbmmx.c:2240
#1 0x00201776 in fbCompositeCopyAreammx (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=3846, xMask=0, yMask=0, xDst=18, yDst=3871, width=920, height=64598) at fbmmx.c:2303
#2 0x001f24fb in fbComposite (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=994, xMask=0, yMask=0, xDst=18, yDst=3871, width=920, height=178) at fbpict.c:1299
#3 0x00247eeb in XAAComposite (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=-926, xMask=0, yMask=0, xDst=18, yDst=789, width=920, height=178) at xaaPict.c:536
#4 0x0070c8d8 in i830_xaa_composite (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=-926, xMask=0, yMask=0, xDst=18, yDst=789, width=920, height=178) at i830_xaa.c:873
#5 0x0815e026 in cwComposite (op=1 '\001', pSrcPicture=0x8cf5030, pMskPicture=0x0, pDstPicture=0x8c304f0, xSrc=0, ySrc=-926, xMsk=0, yMsk=0, xDst=18, yDst=789, width=920, height=178) at cw_render.c:275
#6 0x0815a996 in damageComposite (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=-926, xMask=0, yMask=0, xDst=18, yDst=789, width=920, height=178) at damage.c:541
#7 0x08147b23 in CompositePicture (op=1 '\001', pSrc=0x8cf5030, pMask=0x0, pDst=0x8c304f0, xSrc=0, ySrc=-926, xMask=0, yMask=0, xDst=18, yDst=789, width=920, height=178) at picture.c:1789
#8 0x0814d95f in ProcRenderComposite (client=0x8c6c5c8) at render.c:758
#9 0x0814acd5 in ProcRenderDispatch (client=0xafa4f000) at render.c:2005
#10 0x0808815a in Dispatch () at dispatch.c:459
#11 0x0806fab5 in main (argc=10, argv=0xbfefe174, envp=Cannot access memory at address 0xafa4f008

This also hoses my virtual terminals (ctrl+alt+F1 gives me a white screen) and I have to reboot to get them back.

Version-Release number of selected component (if applicable):
xorg-x11-server-Xorg-1.1.1-48.52.el5

How reproducible:
always

Steps to Reproduce:
1. Grab the latest Firefox 3.5:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-1.9.1/firefox-3.5b4pre.en-US.linux-i686.tar.bz2
2. install it (bzip2 -dc firefox*bz2 | tar -x)
3. Run it (cd firefox; ./firefox)
4. Visit http://www.buffalonews.com/entertainment/moviestv/index.html

==> crash

Thanks for the bug report. We have reviewed the information you have provided above, and there is some additional information we require that will be helpful in our diagnosis of this issue.

Please attach your X server config file (/etc/X11/xorg.conf, if available) and X server log file (/var/log/Xorg.*.log) to the bug report as individual uncompressed file attachments using the bugzilla file attachment link below.

We will review this issue again once you've had a chance to attach this information.

Thanks in advance.

Created attachment 341765
X log

Created attachment 341766
xorg.conf

Created attachment 341775
Xorg-crashing html testcase

X crasher HTML actually doesn't show anything on this RHEL5's firefox-3.0.10-1.el5 and neither the real page crashes (I have flash-plugin installed and Javascript fully on).

Moreover, I don't see anything suspicious in the Xorg.0.log.

Are you able to reproduce this issue with firefox running in the safe mode (i.e. run firefox -safe-mode from the command line)?

Possibly are you able to reproduce this with the upstream binary from mozilla.com?

Thank you very much for filing this bug report and your cooperation with its solving.

> X crasher HTML actually doesn't show anything on this RHEL5's
> firefox-3.0.10-1.el5 and neither the real page crashes

Yes. Only firefox 3.5 (currently in beta) causes a crash.

> I don't see anything suspicious in the Xorg.0.log.

Right. Note that I have 'Option "NoTrapSignals"' in my xorg.conf, which prevents the seg fault notice and mangled stack from showing in the log file (I added it so I could use gdb to get the more useful stack in comment 0).

> Possibly are you able to reproduce this with the upstream binary from
> mozilla.com?

Yes. upstream binaries (see steps to reproduce in comment 0) and builds compiled on RHEL5 both cause the crash.

Does adding: Option "XaaNoOffscreenPixmaps" to the device section of your xorg.conf work around the issue?

yes

Just for the record, I have encountered this problem with the latest version
(I think) of the server, xorg-x11-server-Xorg-1.1.1-48.67.el5, with the page:
http://www.thirdage.com/health-wellness, and the workaround from comment #7,
above has worked for me too.

Created attachment 402099
Possible patch

Looks like the problem occurs because the size passed in fbCompose() to the mmx function are negative (either h_this or w_this is < 0).

The mmx equivalent in fbmmx.c seem to expect a CARD16 for width / height so that might explain the problem.

This patch seems to address the issue at least in my reproducer, but I am not sure of its possible side effects.

Comment on attachment 402099
Possible patch

Patch does not fix all cases, marking obsolete.

Created attachment 403292
Proposed patch

The problem comes from the macro mod() used in computation.

The code in fbComposite() from fbpict.c reads like this:

    if (srcRepeat)
    {
        y_src = mod (y_src - pSrc->pDrawable->y, pSrc->pDrawable->height);
        if (h_this > pSrc->pDrawable->height - y_src)
            h_this = pSrc->pDrawable->height - y_src;
        y_src += pSrc->pDrawable->y;
    }

While inspecting the values, we see that initially, y_src=871, pSrc->pDrawable->y=1024, pSrc->pDrawable->height=500

After computation of mod() y_src=895 (which is wrong) so that h_this = pSrc->pDrawable->height - y_src = -395

Passing a negative value to a CARD16 in mmx function will cause the crash. But the real problem is that the value returned by mod() is actually greater than the values passed which is not possible, so there should be no way that y_src is greater than pSrc->pDrawable->height and therefore h_this should/could not be negative.

mod() is defined as follow (earlier in that code):

# define mod(a,b) ((b) == 1 ? 0 : (a) >= 0 ? (a) % (b) : (b) - (-a) % (b))

Problem is that (-a) gets expanded as "-871 - 1024" (and *not* "- (871 - 1024)" as expected) and therefore "(b) - (-a) % (b)" = 500 - (-871 - 1024) = 895

TI think the following would be more appropriate:

# define mod(a,b) ((b) == 1 ? 0 : (a) >= 0 ? (a) % (b) : (b) - (-(a)) % (b))

That seems to fix the crash and produces the correct output.

Return value seems bit odd for a being a negative multiple of positive b. For positive a, return values range from 0 to b-1, while for negative a, mod returns 1 to b.

True, but the problem the patch is meant to address primarily is the X server crash induced by a wrong macro expansion (rather than changing the macro itself).

For comparison, Pixman uses that definition:

  #define MOD(a, b) ((a) < 0 ? ((b) - ((-(a) - 1) % (b))) - 1 : (a) % (b))

in http://cgit.freedesktop.org/pixman/tree/pixman/pixman-private.h

And uses of that macro for example in walk_region_internal() from http://cgit.freedesktop.org/pixman/tree/pixman/pixman.c which is quite similar to the implementation of the "old" fbComposite()

Devel ack, patch is obviously correct.

*** Bug 570089 has been marked as a duplicate of this bug. ***

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2010-0382.html