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

[Florian Tobias Schandinat <FlorianSchandinat@gmx.de>]

Kai Jiang schrieb:
> 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.)

That's true as my explanation described the problem with the current 
code you encountered.
I also think that your patch will fix it:

> 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;
>        ......
> }

I only wanted to highlight, that as far as I can see the same behavior 
you want to archive can be archived by changing the current code to:

fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
{
        ......
         if (err || !info->fbops->fb_pan_display ||
             var->yoffset > info->var.yres_virtual - yres ||
             var->xoffset > info->var.xres_virtual - info->var.xres)
                 return -EINVAL;
        ......
}

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

I think your patch is fine as it fixes the accepted invalid value.
There are only a few small disadvantages:
- its a bit odd to convert unsigned to signed value to check its validity
- it adds 2 extra compares
- although not practically relevant, as virtual resolutions>2^31 would 
require an enormous amount of video memory, it would be too strict on 
this side (by checking for signedness in u32 you half the range of 
allowed numbers)

I first got Ville Syrjälä second email a bit wrong (sorry for that). He 
suggests to change your check to an overflow check:

fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
{
        ......
         if (err || !info->fbops->fb_pan_display ||
             var->yoffset + yres > info->var.yres_virtual ||
             var->xoffset + info->var.xres > info->var.xres_virtual ||
             var->yoffset + yres < yres ||
             var->xoffset + info->var.xres < info->var.xres)
                 return -EINVAL;
      ......
}

while my approach is to prevent the overflow.
I hope that after my last e-mail you understand, that all 3 suggested 
approaches (yours, mine, Ville Syrjälä) should fix (at least in my 
opinion) your problem. (as negative values don't exist in unsigned types 
or are actually very large positive integers)


Greetings,

Florian Tobias Schandinat

------------------------------------------------------------------------------
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