[Xorg-driver-geode] Xv PutImage crash

[Daniel Drake]

Hi,

At http://dev.laptop.org/ticket/10260 we have a trivial way to make
the geode Xv code crash the X server.
It's probably the same as
http://www.mail-archive.com/xorg-driver-geode@lists.x.org/msg00207.html

I've investigated and have tracked down a buffer overflow, but I'm
confused by the code, not really sure of the fix.

Here's what happens in 1 example:

Xv drops us in LXPutImage() with an I420 image, so we head to LXCopyPlanar().

This function calls LXCopyFromSys() twice. The overflow only happens
(I think) at the end of the 2nd call, but the reason for the overflow
is obvious even when you step into the first one.

So we call LXCopyFromSys(), working on the very start of the Xv
buffer, the image is 512x288, source pitch and dest pitch are both
512. Everything is clear and correct up to here.

We now call gp_set_bpp(16). This is the first questionable thing. The
documentation of gp_set_bpp() says that we're setting the output BPP
of the GP. But surely we should be working at the BPP of the display,
which might not be 16?

A few steps later we arrive in gp_color_bitmap_to_screen_blt().
Continuing with our 512x288 image...

The following happens:

srcoffset = 0
indent = 0

    /* CALCULATE THE SIZE OF ONE LINE */

    size = (width << gp3_pix_shift) + indent;
i.e. size = (512 << 1) + 0 = 1024

    total_dwords = (size + 3) >> 2;
i.e. total_dwords = 256

    size_dwords = (total_dwords << 2) + 8;
i.e. size_dwords = 1032

    dword_count = (size >> 2);
i.e. dword_count = 256

    byte_count = (size & 3);
i.e. byte_count = 0

I'm assuming that we're calculating the size of 1 line of the input
image (or that we should be).

These calculations are suspicious to me. gp3_pix_shift is set to 1 by
the earlier call to gp_set_bpp(), which, as mentioned above, is
supposedly about the output BPP, theoretically unrelated to the input
image that we're dealing with.

Continuing on, this is not a "small BLT case" so we fall through to
the "else" condition.

We arrive in a loop which looks like it should run line by line of the
input image:
        while (height--) {

Then we write the image data from the buffer to the hardware:

            /* WRITE DWORD COUNT */

            WRITE_COMMAND32(4, GP3_HOST_SOURCE_TYPE | total_dwords);

            /* WRITE DATA */
            WRITE_COMMAND_STRING32(8, data, srcoffset, dword_count);
            WRITE_COMMAND_STRING8(8 + (dword_count << 2), data,
                srcoffset + (dword_count << 2), byte_count);

            /* UPDATE POINTERS */

            srcoffset += pitch;


Looking carefully: we're saying that for each line of the input image,
we're going to write 256 dwords to the hardware (256 is the value of
total_dwords and also dwords_count).

But, 256 dwords is 1024 bytes, but the pitch is 512.
We're writing 2 lines, not 1!

Then we increment srcoffset for the next loop iteration, but we
iterate it by just 512.

So, if we name the first 4 rows of the image A, B, C and D, on the
first loop iteration we write A and B, on the second iteration we
write B and C, on the third we write C and D.

The overflow happens when we're writing the final line, because we
write the final line and the line that comes afte...oh no, we've
overrun the buffer.

It seems to me that the calculation of "size" (and hence
total_dwords/dword_count) should consider the pitch of the input
image, rather than the BPP set by gp_set_bpp(). This solves the crash:

    /* CALCULATE THE SIZE OF ONE LINE */
    size = pitch + indent;

But I'm also a bit mystified by gp_set_bpp() in this context. And
looking at the other blt functions in the file just confuses me
further.

Any ideas/comments?

Thanks,
Daniel