Re: [linux-fbdev-devel][PATCH]fb_pan_display:add x/yoffset check

Florian Tobias Schandinat wrote:

Kai Jiang schrieb:

quoted

Florian Tobias Schandinat wrote:

quoted

That's true, but the problem lies in the current implementation 
first adding the resolution, which results in small negative [0 to 
-resolution] values (=large positives) being accepted as they 
overflow during add and become small positive values.
I'd recommend changing

var->yoffset + yres > info->var.yres_virtual ||
var->xoffset + info->var.xres > info->var.xres_virtual

to

var->yoffset > info->var.yres_virtual - yres ||
var->xoffset > info->var.xres_virtual - info->var.xres

I am not sure why do we have these change. Could you give a detail 
description or an example?

It starts with "-1" in an u32 being represented as "0xFFFFFFFF", which 
would be caught by ">". The problem in the current code is it first 
adds the resolution before comparison and this causes an overflow.
Let's say the virtual resolution matches the real resolution:
yoffset + yres > yres
There the left side is evaluated at first:
(yoffset + yres)
You accept everything that is <=yres. In classical mathematics you 
would say yoffset has to be 0, but unfortunately this codes accept 
many more as it can overflow. You get
yoffset = -1:    (yres-1) > yres
offset = -yres:    0 > yres
So as you noticed, the current code will not just accept 0 as yoffset, 
but the whole range [-yres..0]. This can be fixed by moving the 
calculation to the right side, where we have trusted values, that do 
not cause an overflow.

Florian Tobias Schandinat,
Thank you for your quick detail reply.
While, I suppose when the patch is applied, it should avoid what you 
mentioned. Following is the code applied patch.
(And the x/yres and x/yres_virtual have fix value which are defined and 
checked in the driver.)

fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
{      ......
        int xoffset = var->xoffset;                 // here transfer 
x/yoffset to "int" type for comparison
        int yoffset = var->yoffset;
       ......
        if (err || !info->fbops->fb_pan_display ||
            var->yoffset + yres > info->var.yres_virtual ||        
            var->xoffset + info->var.xres > info->var.xres_virtual ||    
            xoffset < 0 || yoffset < 0)             // insure the 
x/yoffset is large than 0. I think this line can avoid what you concerned.
                return -EINVAL;
       ......
}

Do you think so? I am happy to know your comments.

Best Regards,

Kai Jiang




------------------------------------------------------------------------------
Enter the BlackBerry Developer Challenge  
This is your chance to win up to $100,000 in prizes! For a limited time, 
vendors submitting new applications to BlackBerry App World(TM) will have 
the opportunity to enter the BlackBerry Developer Challenge. See full prize 
details at: http://p.sf.net/sfu/blackberry