Bug 13520

Created attachment 12941 [details]
Draft advisory

Created attachment 13034 [details] [review]
Proposed fix

okay please drop the first hunk of ajax's patch, its buggy as size is replaced in the derference..

further down 
ProcPanoramiXShmCreatePixmap
contains the same code does this also require a fix?

Created attachment 13099 [details] [review]
proposed fix - for both functions..

Okay this fixes both functions non-panoramix and panoramix..

Created attachment 13178 [details]
Testcase

This is a test for the simplest case (width & height of 32768, depth of 32 bit =
32768 * 32768 * 4 = INT_MAX+1), which is allowed to pass before the fix, and 
now caught by the simple width > 32767 & depth > 32767 checks - better tests for 
the cases in the more complex code paths would be good to have too.

This is public now

Patch has beem committed: 6de61f82728df22ea01f9659df6581b87f33f11d

Created attachment 13779 [details]
mitshm-testcase.c

Enhanced testcase that also verifies that 1bpp pixmaps continue to work, ie, that:

http://cgit.freedesktop.org/xorg/xserver/commit/?id=e9fa7c1c88a8130a48f772c92b186b8b777986b5

does the right thing.

+    size = PixmapBytePad(width, depth) * height;
+    if (sizeof(size) == 4 && BitsPerPixel(depth) > 8) {
                                                 ^^^
IMHO this should read >=
Currently this *doesn't* test 8bpp pixmaps.

+        if (size < width * height)
+            return BadAlloc;

I *really* don't understand this test. When can this ever hit true (with depth >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767 perfectly fits 32bit.

+        /* thankfully, offset is unsigned */
+        if (stuff->offset + size < size)
+            return BadAlloc;

This overwrap test (which I assume was the original fix) can (and should) always be done, independently of the machine size and pixel depth.

Agreed?

(In reply to comment #9)
> +    size = PixmapBytePad(width, depth) * height;
> +    if (sizeof(size) == 4 && BitsPerPixel(depth) > 8) {
>                                                  ^^^
> IMHO this should read >=
> Currently this *doesn't* test 8bpp pixmaps.

Sure, but everybody stores depth 8 pixmaps as 8bpp, and as you note below, 32767*32767 < 2^32-1.  It'd be harmless to test 8bpp too, I suppose.

> +        if (size < width * height)
> +            return BadAlloc;
> 
> I *really* don't understand this test. When can this ever hit true (with depth
> >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767
> perfectly fits 32bit.

Yeah, for <=32bpp it's fine.  I suppose it's just paranoia in case anyone starts doing >32bpp, but that day is soon.

> +        /* thankfully, offset is unsigned */
> +        if (stuff->offset + size < size)
> +            return BadAlloc;
> 
> This overwrap test (which I assume was the original fix) can (and should)
> always be done, independently of the machine size and pixel depth.
> 
> Agreed?

Agreed.

> > +        if (size < width * height)
> > +            return BadAlloc;
> > 
> > I *really* don't understand this test. When can this ever hit true (with depth
> > >=8, that is)? size, width, and height are unsigned 32bit, and 32767*32767
> > perfectly fits 32bit.
> 
> Yeah, for <=32bpp it's fine.  I suppose it's just paranoia in case anyone
> starts doing >32bpp, but that day is soon.

Hm. Yes, sounds true. But wouldn't be the proper fix to just change size to CARD64? AFAICS the structure is used in this file only...

Pushed the offset wrapping in be6c17fcf9e.