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

[Kai Jiang <b18973@freescale.com>]

Florian Tobias Schandinat wrote:
> Kai Jiang schrieb:
>> Florian Tobias Schandinat wrote:
>>> 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