[Xorg-driver-geode] Xv PutImage crash

[Cui, Hunk]

Hi, Daniel,

	My platform is Ubuntu 10.04, Xserver version is 1.8.99,
xf86-video-geode version is 2.11.8.
	I have tested http://olpc.dailymotion.com/, then clicked on the
second-from-left image ('Un jour ... a Paris ...' - the one with Walter
Bender's face), The file started to be shown, and the video can normally
broadcast, The input was accepted from keyboard and the USB mouse can be
used.
	Could you provide your Xorg.0.log and xorg.org file please? And
tell me what the system version is. 
	Maybe you'd like to join our IRC channel #geode on FreeNode
network or newly establish bugzilla a bit for Driver/geode component
bugs :) (https://bugs.freedesktop.org). I'll appreciate to do this.
	Any information, please inform me.

Thanks,
Hunk Cui

> -----Original Message-----
> From: xorg-driver-geode-bounces+hunk.cui=amd.com at lists.x.org
[mailto:xorg-
> driver-geode-bounces+hunk.cui=amd.com at lists.x.org] On Behalf Of Daniel
Drake
> Sent: Tuesday, August 03, 2010 5:07 AM
> To: xorg-driver-geode at lists.x.org
> Subject: [Xorg-driver-geode] Xv PutImage crash
> 
> 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
> _______________________________________________
> Xorg-driver-geode mailing list
> Xorg-driver-geode at lists.x.org
> http://lists.x.org/mailman/listinfo/xorg-driver-geode