Bug 29391

Summary: Xv PutImage buffer overflow when handling I420 images
Product: xorg Reporter: Daniel Drake <dan>
Component: Driver/geodeAssignee: Xorg Project Team <xorg-team>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium    
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
fix
none
fix Daniel Stone's concern none

Description Daniel Drake 2010-08-03 15:48:13 UTC 
When using Xv to display an I420 image, a buffer overflow is triggered.

http://lists.x.org/archives/xorg-driver-geode/2010-August/000938.html

The problem is that gp_color_bitmap_to_screen_blt() does not consider pitch of the input image when it is doing the copy. It always assumes 2 bytes per pixel, as determined by the leading (but erroneous?) gp_set_bpp(16) call.

For YUYV images there is no problem (2 bytes per pixel are copied). For I420 images, the image planes that this function has to copy are 1 byte per pixel, so the function ends up copying twice the amount of data, and reading beyond the end of the buffer.

Fixing the function to consider image pitch (a parameter already available in the function) fixes the bug, but introduces a regression where another of my videos causes the geode driver to hang (while waiting for the blt engine to stop being busy). It is possibly related to the fact that this 2nd video is low-resolution (160x120).

Analysis can be found in the thread linked to above.

Version details are not too relevant as I've tracked down the buggy code in geode git HEAD, but anyway:

OLPC XO-1 running OLPC OS 10.1.2 beta (based on Fedora 11)
X server 1.6.4 RC1 
xf86-video-geode git HEAD as of today

Xorg.0.log: http://dev.laptop.org/~dsd/20100803/Xorg.0.log
xorg.conf: http://dev.laptop.org/~dsd/20100803/xorg.conf

Video that I'm using to trigger the crash relatively easily:
http://proxy-48.dailymotion.com/video/791/905/21509197%3aogg_theora_vorbis%3a255.ogg?auth=1280882838-c862f9f6c8fd3de117a4e04b3676246c&cache=0

Low resolution video that causes a hang when the obvious fix is applied:
http://dev.laptop.org/~dsd/20100803/video.ogv
Comment 1 Daniel Drake 2010-08-04 19:44:09 UTC 
Created attachment 37578 [details] [review]
fix

Fixed it.

The set_bpp call is valid. We're BLTing an image from system memory to video memory. We aren't displaying it on the screen; the screen output BPP is irrelevant. As we're copying without transforming the data in any way, the BLT output BPP needs to be the same as the input image.

Therefore the bug is that LXCopyFromSys currently expects 16bpp input data, but is called with 8bpp planes in the LXCopyPlanar path. This patch fixes it by deriving the bpp from the input parameters.
Comment 2 Daniel Stone 2010-08-04 22:49:26 UTC 
On Wed, Aug 04, 2010 at 07:44:10PM -0700, bugzilla-daemon@freedesktop.org wrote:
> Fixed it.
> 
> The set_bpp call is valid. We're BLTing an image from system memory to video
> memory. We aren't displaying it on the screen; the screen output BPP is
> irrelevant. As we're copying without transforming the data in any way, the BLT
> output BPP needs to be the same as the input image.
> 
> Therefore the bug is that LXCopyFromSys currently expects 16bpp input data, but
> is called with 8bpp planes in the LXCopyPlanar path. This patch fixes it by
> deriving the bpp from the input parameters.

Um, stride in bytes / width in pixels isn't always bpp, given various
alignment constraints and the need for padding.  You'll just have to
pass a bpp/format in explicitly.
Comment 3 Martin-Éric Racine 2010-08-23 08:17:52 UTC 
Unless I'm mistaken, this is fixed by commit e9effca821c1d604aeffeb3d3e7a49539485117d in Geode 2.11.9 that was just released?
Comment 4 Daniel Drake 2010-09-06 17:28:58 UTC 
Created attachment 38497 [details] [review]
fix Daniel Stone's concern

additional patch to address Daniel's concern above
Comment 5 frank huang 2010-09-28 19:21:45 UTC 
Close this bug now that it has been fixed.

Thanks,
Frank

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.