Bug 7447

Sorry about the phenomenal bug spam, guys.  Adding xorg-team@ to the QA contact so bugs don't get lost in future.

Move to 7.3 tracker.

The issue appears to be caused by compNewPixmap.  I was under the impression that this function copied the contents of the old pixmap into the new one.  However, what it actually does is copy the contents of the *parent window* into the new pixmap.  When the parent window is depth 32 and the child pixmap is depth 16, this overruns the pixmap by a factor of 2.

Created attachment 11157 [details] [review]
Proposed fix

I was wrong: It ends up in fb24_32CopyMtoN so it overruns the destination pixmap by a factor of 3/2 rather than 2.

Anyone got any objections to an embargo until August 29th? Might as well do it at the same time.

And here I was thinking I was getting more competent with Bugzilla and security.

Comment on attachment 11157 [details] [review]
Proposed fix

That patch is totally busted.  I don't know what I was thinking.  I'll fix it tomorrow.

Basically, the copyarea just has to be replaced with a composite, right?

That would work, though it would be slower.

Created attachment 11368 [details] [review]
Use Composite when depths don't match

We'll use Composite when the depths don't match. This patch seems obvious enough. I haven't tested it myself though.

That patch works for me.

well, the patch is now in git, so i guess we should drop any pretense of secrecy.

does anyone feel like backporting the fix?

i'm having what i believe to be the same problem, caught internally (we use a 16-bit server):
STEPS TO REPRODUCE

launch xcompmgr &
run attached testcase and provide it '-f' as parameter.

------- Comment #5 From Stone Daniel (Nokia) 2007-08-17 09:49 [reply] -------

Strange, this doesn't happen in Xephyr.

------- Comment #6 From Palli Tapani (Nokia) 2007-08-17 10:24 [reply] -------

(In reply to comment #5)
> Strange, this doesn't happen in Xephyr.
> 

Yep, does not happen in Xephyr for me aswell :/ ... please try on target.

------- Comment #8 From Tamminen Eero (Nokia) 2007-08-17 11:22 [reply] -------

(In reply to comment #6)
> > Strange, this doesn't happen in Xephyr.
> 
> Yep, does not happen in Xephyr for me aswell :/

If you Valgrind Xephyr, do you get any errors?
(the error could be there, but causing crash only on arm)

------- Comment #9 From Stone Daniel (Nokia) 2007-08-17 13:43 [reply] -------

This one's not going to be much fun.  We die inside ValidateGC (specifically,
the composite wrapper), despite the ShmPutImage arguments being exactly the
same.  When setup comes, we do everything right:
Breakpoint 2, cwSetWindowPixmap (pWindow=0x2be9a0, pPixmap=0x2a7ef8)
    at ../../../miext/cw/cw.c:610
610     in ../../../miext/cw/cw.c
(gdb) print pWindow->drawable.id
$4 = 4194316 [note: this is 0x40000c]

But by the time we come down into ShmPutImage, we've lost out badly:
shm put image: format 2, depth 16, drawable 40000c, gc 40000d, shmseg 40000e, 
offset (nil), width 800, height 480, srcx 0, srcy 0, srcw 800, srch 480
Program received signal SIGSEGV, Segmentation fault.
0x001cc390 in cwGetBackingDrawable (pDrawable=0x2be9a0, x_off=0xbeb97f98,
    y_off=0xbeb97f94) at ../../../miext/cw/cw.c:96
96      in ../../../miext/cw/cw.c
(gdb) print pDrawable.id
$9 = 4194316
(gdb) print ((WindowPtr) pDrawable)->devPrivates[cwWindowIndex].ptr             
$28 = (pointer) 0x404ce008

(Note that x_off and y_off are pointers for return values, not offsets outside
the co-ordinate limits.)

 ------- Comment #11 From Stone Daniel (Nokia)  2007-09-03 12:56  [reply] -------

only this:
==17151== 
==17151== Conditional jump or move depends on uninitialised value(s)
==17151==    at 0x403D64F: pixman_region_translate (pixman-region.c:2017)
==17151==    by 0x80D556A: miTranslateRegion (miregion.c:1586)
==17151==    by 0x8388002: compSetRedirectBorderClip (compwindow.c:647)
==17151==    by 0x80D9BCC: miComputeClips (mivaltree.c:258)
==17151==    by 0x80DA67F: miValidateTree (mivaltree.c:770)
==17151==    by 0x80869E4: MapWindow (window.c:2841)
==17151==    by 0x8096975: ProcMapWindow (dispatch.c:740)
==17151==    by 0x80F8440: XaceCatchDispatchProc (xace.c:281)
==17151==    by 0x809712E: Dispatch (dispatch.c:502)
==17151==    by 0x807F2E4: main (main.c:452)

only happens with testcase -f, not testcase.  (note that the offending code is
in miTranslateRegion in our codebase, not pixman_translate_region.)

Created attachment 11397 [details]
testcase for crasher

Running with no arguments and xcompmgr running on a 16bpp server is fine, but using -f causes the server to die.

cherry-picked to server-1.4

daniels, I failed to reproduce your issue, with or without the cherry-pick, with a 16-bit XAA server.

This has been assigned CVE-2007-4730, according to http://lists.debian.org/debian-security-announce/debian-security-announce-2007/msg00134.html

I realize this bug is fixed, but I have to ask if there could be another way to close the security hole.  As it is now, what Aaron Plattner said "That would work, though it would be slower." is far beyond true.  It's to the point where it's impractical to use any composite aware terminal with compositing on.  

Is there any way to avoid using compiz for the case of differing depths, or possibly to set the default depth for all windows to be 32 when compiz is active even if the window is not using all 32 bits?

If need be I can provide more information, but the heart of it is that I remember a time when a composited terminal was faster than non, but now my newer system with much faster graphics (geforce 8800gt vs geforce fx5200) can't display a terminal emulator without lagging... doesn't seem to sit right somehow.

I'd be happy to work on developing a patch, but I'm admittedly new to X11 development, so I wanted to ask you, the experts, whether there's something which can be done before I sink time into it.