Bug 594

Created attachment 259 [details]
The postscript file

gv-3.5.8-24
xorg-x11-0.0.6.6-0.0.2004_03_11.11

(FC2-Test2)

Actually for me it nuked X instantly.  Worse yet, it kills Xorg but not Xnest. 
I'll try to get either a core dump or a protocol trace.

Verrry interesting.  I can't get a protocol trace of the action, because if I
run gv with DISPLAY=127.0.0.1:0, it doesn't crash, and there's no way to capture
on a unix-domain socket.  However, Xnest will not crash, regardless of whether
gv connects to it over TCP or UDS.

This, combined with the gv display when it doesn't crash (a ~gigapixel black
rectangle) leads me to suspect an integer overflow somewhere in the MIT-SHM
code.  SHM isn't active over TCP, which would explain why I can't get a protocol
dump of the crash.  Xnest also does not do MIT-SHM (on my machine anyway; it
should I would think).  Finally, huge integers used as pointer offsets would
easily cause a segfault and ensuing instant crash.

A fairly wild guess, but that probably needs auditing anyway...

assigning to me

luke, what video card are you using?  trying to determine if this is
driver-dependent or not.

(In reply to comment #6)
> luke, what video card are you using?  trying to determine if this is
> driver-dependent or not.

I think the machine I reported this on was my work machine, which has an nVidia
FX5200 chipset.  It was using the nv driver, I think.  The other machine it
could have been is also an nVidia (TNT2), using the vesa driver (although I'm
pretty sure it was the former machine).

My laptop has an Intel 810/855 chipset, and it doesn't crash at all (Driver:
"i810").  So you might be onto something.  Although the laptop has the final FC2
with all the latest updates installed, if that makes a difference -- the machine
with the crash was FC2test2.

yeah, it seems to be build-dependent here too.  i can't reproduce it in debrix
at all, or on my mach64, but Xorg + tdfx blows right up.

what's the package info for Xorg from FC2 final?  maybe they've got a patch that
addresses it...

# rpm -qa | grep xorg
xorg-x11-Mesa-libGL-6.7.0-2
xorg-x11-devel-6.7.0-2
xorg-x11-tools-6.7.0-2
xorg-x11-libs-data-6.7.0-2
xorg-x11-libs-6.7.0-2
xorg-x11-Mesa-libGLU-6.7.0-2
xorg-x11-base-fonts-6.7.0-2
xorg-x11-xfs-6.7.0-2
xorg-x11-twm-6.7.0-2
xorg-x11-font-utils-6.7.0-2
xorg-x11-xauth-6.7.0-2
xorg-x11-6.7.0-2
xorg-x11-100dpi-fonts-6.7.0-2
xorg-x11-75dpi-fonts-6.7.0-2

Is this helpful?

crasher, bumping severity.

i've finally got the dlloader build mostly-working, let's see if i can't get a
core dump...

Created attachment 520 [details]
no core dump, but got a working backtrace at least...

in frame 0 at the point of the crash, src is out of bounds.  the width
parameter to fbBlt looks completely bogus...

Keith what do you think?

Well, that was fun...

Looks like fb needs to range check arguments to CreatePixmap to avoid pixmaps
which can't be represented within a Region.  I recall fixes like this in other
areas of the server in the past; perhaps those never made it back into the
sample implementation from various commercial copies?  

(And people wonder what's wrong wit h the MIT license...)

I have tried to reproduce this with the current CVS head and cannot.  It takes
over 10 minutes to display, but it works fine.  Keith suggested that the problem
might be caused by running out of memory as gv allocates a tremendously large
pixmap for this postscript file (RSS > 800M).

This was discussed on today's release wranglers call, and we decided that this
bug would not block the release unless a problem, other than the memory
management or range checking issues mentioned above, can be confirmed.

However, we agree that there are issues surrounding the pixmap size and what can
be legally represented in a region.  These issues are known issues and are being
deferred until after the release.  However, they do need to be documented for
this release.

Moving bug over to the bug 999 for documentation.

Created attachment 3008 [details]
A standalone reproducing app

The attached program crashes my X server.

CAN-2005-2495, embargoed until 2005-09-08, 1400 UTC.

Created attachment 3072 [details] [review]
patch against 4.3?

I think this patch is against 4.3.x; it's to Søren's mail to xorg_security@.

+    if (paddedWidth / 4 > 32767 || height > 32767)
+	return NullPixmap;
     datasize = height * paddedWidth;

I'm trying to understand why we are dividing paddedWidth by 4.
A paddedWidth of 32766*4 and a height of 32766 would be legal.
datasize is int. 32766 * 32766*4 is 0xfff80010 - which is more than you can fit
into an int.
Where does the 4 come from?
Also I don't see why we check for depth > 4 for ilbm and afb. Shouldn't this be
caught elsewhere?

embargo extended to 2005-08-12 1400 UTC per xorg_security and vendor-sec

2005-09-12, of course

embargo over, welcome to full disclosure city.

Comment on attachment 3072 [details] [review]
patch against 4.3?

y'know, if we're serious about the whole 6.8.3 thing, this one sounds like a
shoo-in

committed søren's patch to head

To round this up here is the explanation for the devision by 4 in:
+    if (paddedWidth / 4 > 32767 || height > 32767)
+	return NullPixmap;

We must meet the condition:
paddedWidth * height <= ((1 << (sizeof(size_t) << 3)) - 1)
so that we don't cause an overflow in the argument of a subsequent malloc()
and end up allocating less than the required amount.
size_t should be at least unsigned int therefore the maximum value it can hold
would be: 32767^2 * 4. height needs to be <= 32767 as it needs to fit into a
short. This condition should have been tested in the calling function - we do it
here again for safety.