Bug 431191

Related discussion in the original upstream bug report for MIT-SHM overflow:

https://bugs.freedesktop.org/show_bug.cgi?id=13520#c9

Yeah, should pull this in.

On reflection, this subsequent change is unnecessary.

One bit of seemingly irrelevant trivia before we get started.  The maximum size
of a SysV shared memory segment is controlled by /proc/sys/kernel/shmmax.  In
rawhide this is 32M; I don't believe it was ever much larger than this.

The issue in the original CVE is about this macro:

#define VERIFY_SHMSIZE(shmdesc,offset,len,client) \
{ \
    if ((offset + len) > shmdesc->size) \
    { \
        return BadAccess; \
    } \
}

shmdesc is a descriptor of the SysV shared memory segment holding the image
data, and ->size is the size in bytes.  offset is the (byte) offset into the shm
segment where the image data starts, clamped to ->size by earlier validation
code.  len is a derived value based on the width, height, and depth of the image
data, which describes the byte length of the data, taking all relevant padding
into account.  offset is uint32 from the wire protocol, len is however big it
needs to be according to C promotion rules (though we'll cover that in a
second), and size is unsigned long.

Conveniently, len is always representable in a uint32.  The maximum pixel
dimensions of a pixmap in X are 32767 by 32767, and the maximum number of bytes
per pixel is 4.  4294705156 < 4294967295.  The fix, as initially committed,
correctly clamps the width and height values to 32767 each, and the maximum
depth (in bits) to 32, so we know len will fit in a uint32.

Therefore, on LP64 platforms, we're done.  size is uint64, and the sum of two
uint32s can not possibly overflow a uint64.  The remainder of this discussion is
relevant for ILP32 platforms only.

At this point, having validated width/height/depth, we compute size:

    size = PixmapBytePad(width, depth) * height;

PixmapBytePad(), in the worst case, pads out to the nearest multiple of 32.  So
size is now the size in bytes of the image; but this size could still be _huge_,
such that adding offset to it would wrap around.  So we check:

    if (sizeof(void *) == 4 && BitsPerPixel(depth) > 8) {
        if (size < width * height)
            return BadAlloc;
        /* thankfully, offset is unsigned */
        if (stuff->offset + size < size)
            return BadAlloc;
    }

The outer conditional checks that we're on ILP32 and that we are using at least
one byte per pixel for storage.  If so, then we check two things.  First we test
for the edge case where PixmapBytePad(width, depth) ends up larger than 131076,
which would overflow, and thus make size very small.  Second, we check for
overflow when adding offset to size.  So in either case here, we are correctly
protected; if the requested dimensions from the client, plus the offset into the
shm segment, would wrap around in process memory, we just refuse.

So far, there's no contention.  The modification in
https://bugs.freedesktop.org/show_bug.cgi?id=13520#c9 moves the (stuff->offset +
size < size) check outside the check for BitsPerPixel(depth) < 8.  While valid,
this is meaningless.  BitsPerPixel(depth) < 8 only for depth == 1.  This is the
case of a bitmap, literally only one bit per pixel.  In this case the maximum
allocation is 32767 * 32767 bits (plus right padding out to the next multiple of
32 bits), which is a mere 128M.  Since offset is clamped to size by earlier
validation code, and size is clamped to 32M by system policy, there is no
possible way that (stuff->offset + size) can be > 160M, which is a long long way
from overflowing.

There is one final possibility, which is that the shm segment for this case
could have been attached between 128 and 160M of the top of address space.  This
is also a non-issue, userspace processes occupy the low 3G of address space.

In short, the proposed modification does not protect against any new overflow
scenario.

Value of /proc/sys/kernel/shmmax may be set by system administrator to much
higher value than default of 32M.  To cause an overflow using depth == 1
pixmaps, which are not checked by currently used patch, attacker would have to
create request with SHM segment (stuff) of size (4G - 128M) and offset near the
end of that segment (stuff->offset) to cause an overflow.

It's quite unlikely that shmmax is set to a value that would allow such large
SHM segments.  Additionally, standard 32bit kernels only have 3G virtual address
space for applications, creating additional limit on maximum SHM segment size. 
So this may only be an issue on systems with shmmax set to ~4G and kernels with
4G-4G userspace-kernel address split.