Bug 21666

Summary: Logic error in i830_valid_command
Product: xorg Reporter: Alan Coopersmith <alan.coopersmith>
Component: Driver/intelAssignee: Carl Worth <cworth>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: low    
Version: git   
Hardware: x86 (IA32)   
OS: All   
Whiteboard:
i915 platform: i915 features:

Description Alan Coopersmith 2009-05-10 15:57:27 UTC
While running the parfait source checker over the X.Org sources, it
returned a warning of:

Error: Read buffer overflow at xf86-video-intel-2.6.1/src/i830_debug.c:1380 in function 'i830_valid_command' [Symbolic analysis]
    In array dereference of _3d_cmds[pipeline_type] with index 'pipeline_type'
       Array size is 4 elements (of 16384 bytes each), index >= 4

The relevant code in git master (starting at line 1485 now) is:
        pipeline_type = (cmd >> 27) & 0x3;
	[...]
        if (pipeline_type <= 3)
            return count;
        if (!_3d_cmds[pipeline_type][opcode][subopcode].name)
            return -1;

It seems like the first if may be reversed and should be >= 3, though
> 3 should be impossible given the & 0x3 in the assignment.

[ This bug was found by the Parfait bug checking tool. 
    For more information see http://research.sun.com/projects/parfait ]
Comment 1 Wang Zhenyu 2009-07-30 01:04:44 UTC
It seems in case pipeline_type is sane, just return the count number. We might simply remove "if (pipeline_type <= 3) return count;" for cleanup.

Comment 2 Carl Worth 2009-07-31 11:27:51 UTC
Thanks for the report, Alan.

It's easy to see how this code would confuse parfait. It confused us as well. :-)

It seems fairly clear to me that the intent was to determine -1 if the command table has no name for the command being referenced. And the early return, (which was made unconditional based on the masking as you mentioned was preventing that).

My inclination is to just remove the early return as Zhenyu mentions, (and as the patch below implements).

But perhaps the early-return was meant to be a buffer-overflow protection, (which does seem a bit silly coming two-lines after the masking), so it would be legitimate to switch the sense of the condition as well.

Anyway, I've posted my patch and I'll close this report when the patch is aplied.

-Carl

commit 2c19210b9d0224e61016d8cc76e1af28632fc0c1
Author: Carl Worth <cworth@cworth.org>
Date:   Fri Jul 31 11:20:23 2009 -0700

    debug: i830_valid_command: Return invalid for subopcodes with no name
    
    Previously the code would always return the count, before ever looking
    into the _3d_cmds table to see if there was actually a valid command.
    
    Thanks to Alan Coopersmith who reported that the code was confusing
    parfait:
    
    https://bugs.freedesktop.org/show_bug.cgi?id=21666

diff --git a/src/i830_debug.c b/src/i830_debug.c
index 1126c26..4b704a1 100644
--- a/src/i830_debug.c
+++ b/src/i830_debug.c
@@ -2094,8 +2094,6 @@ i830_valid_command (uint32_t cmd)
            count = 1;
        else
            count = (cmd & 0xff) + 2;
-       if (pipeline_type <= 3)
-           return count;
        if (!_3d_cmds[pipeline_type][opcode][subopcode].name)
            return -1;
        break;
Comment 3 Chris Wilson 2010-05-11 09:47:31 UTC
The code has since been deleted in its entirety.

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.