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

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

[Kai Jiang]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: fb_pan_display-check-offset.patch --]
[-- Type: text/x-patch, Size: 1811 bytes --]

From a01ede69772634b30a83b44eada5a8db66f8463a Mon Sep 17 00:00:00 2001
From: Kai Jiang <Kai.Jiang@freescale.com>
Date: Mon, 29 Jun 2009 11:25:58 +0800
Subject: [PATCH] When moving virtual space straight to one side in the screen(ex.
 straight to the left),finally the virtual space will move outside
 of the real screen. Then the xoffset or yoffset will be nagative
 value(transfered from user application) to indicate that the virtual
 space is beyond the screen boundary. In the function fb_pan_disaplay,
 xoffset and yoffset should be checked to ensure that, when they are
 negative, the virtual space will not move any more,and the function
 will return an error. However, xoffset and yoffset in the structure
 fb_var_screeninfo are "__u32" type, here need to transfer them to
 "int" type for comparing.

Signed-off-by: Kai Jiang <Kai.Jiang@freescale.com>
---
 drivers/video/fbmem.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index d412a1d..27628de 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -855,6 +855,8 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
 {
 	struct fb_fix_screeninfo *fix = &info->fix;
 	unsigned int yres = info->var.yres;
+	int xoffset = var->xoffset;
+	int yoffset = var->yoffset;
 	int err = 0;
 
 	if (var->yoffset > 0) {
@@ -873,7 +875,8 @@ 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->xoffset + info->var.xres > info->var.xres_virtual ||
+	    xoffset < 0 || yoffset < 0)
 		return -EINVAL;
 
 	if ((err = info->fbops->fb_pan_display(var, info)))
-- 
1.6.2.1


[-- Attachment #3: Type: text/plain, Size: 79 bytes --]

------------------------------------------------------------------------------

[-- Attachment #4: Type: text/plain, Size: 182 bytes --]

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

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

[Ville Syrjälä]

On Mon, Jun 29, 2009 at 11:49:31AM +0800, Kai Jiang wrote:
>

> >From a01ede69772634b30a83b44eada5a8db66f8463a Mon Sep 17 00:00:00 2001
> From: Kai Jiang <Kai.Jiang@freescale.com>
> Date: Mon, 29 Jun 2009 11:25:58 +0800
> Subject: [PATCH] When moving virtual space straight to one side in the screen(ex.
>  straight to the left),finally the virtual space will move outside
>  of the real screen. Then the xoffset or yoffset will be nagative
>  value(transfered from user application) to indicate that the virtual
>  space is beyond the screen boundary. In the function fb_pan_disaplay,
>  xoffset and yoffset should be checked to ensure that, when they are
>  negative, the virtual space will not move any more,and the function
>  will return an error. However, xoffset and yoffset in the structure
>  fb_var_screeninfo are "__u32" type, here need to transfer them to
>  "int" type for comparing.
> 
> Signed-off-by: Kai Jiang <Kai.Jiang@freescale.com>
> ---
>  drivers/video/fbmem.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index d412a1d..27628de 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -855,6 +855,8 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
>  {
>  	struct fb_fix_screeninfo *fix = &info->fix;
>  	unsigned int yres = info->var.yres;
> +	int xoffset = var->xoffset;
> +	int yoffset = var->yoffset;
>  	int err = 0;
>  
>  	if (var->yoffset > 0) {
> @@ -873,7 +875,8 @@ 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->xoffset + info->var.xres > info->var.xres_virtual ||
> +	    xoffset < 0 || yoffset < 0)

Well negative xoffset/yoffset don't really exist so what you're
essentially checking is whether offset+res overflows. Your check will
not catch all overflows though. xres/yres would have to be huge
(> 2^31) to cause such overflows though so your check should catch all
cases that can happen in practice. However I think it would be better
to make the overflow check clearer (eg. 'offset + res < res').

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------

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

[Kai Jiang]

xres,yres,xres_virtual,yres_virtual will be set in the display driver, they have actual value which match the screen(will not very huge).

And here, these lines are not checking the overflowing, but checking whether the virtual space display is beyond the real screen.

Take x boundary for example:(the screen x and virtual x relationship should be:)
xres+xoffset<xres_vritual && xoffset>0
This insure that, in the real screen, there are virtual picture display in the screen. When we move the virtual picture to the left, the xoffset will be smaller and smaller
to indicate the virtual picture is moving to the left. Finally, the xoffset will be zero when the screen left boundary and virtual picture left boundary overlap. When virtual display is still moving to the left, the xoffset will be negative. And what should be displayed in the gap between screen left boundary and virtual space left boundary?

So here we have to check the whether the x/yoffset is smaller than zero. If the offset is smaller than zero, in the driver, we should not move the virtual screen any more.


Best Regards,

Kai Jiang
 
 


> On Mon, Jun 29, 2009 at 11:49:31AM +0800, Kai Jiang wrote:
>   
>
>   
>> >From a01ede69772634b30a83b44eada5a8db66f8463a Mon Sep 17 00:00:00 2001
>> From: Kai Jiang <Kai.Jiang@freescale.com>
>> Date: Mon, 29 Jun 2009 11:25:58 +0800
>> Subject: [PATCH] When moving virtual space straight to one side in the screen(ex.
>>  straight to the left),finally the virtual space will move outside
>>  of the real screen. Then the xoffset or yoffset will be nagative
>>  value(transfered from user application) to indicate that the virtual
>>  space is beyond the screen boundary. In the function fb_pan_disaplay,
>>  xoffset and yoffset should be checked to ensure that, when they are
>>  negative, the virtual space will not move any more,and the function
>>  will return an error. However, xoffset and yoffset in the structure
>>  fb_var_screeninfo are "__u32" type, here need to transfer them to
>>  "int" type for comparing.
>>
>> Signed-off-by: Kai Jiang <Kai.Jiang@freescale.com>
>> ---
>>  drivers/video/fbmem.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index d412a1d..27628de 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -855,6 +855,8 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
>>  {
>>  	struct fb_fix_screeninfo *fix = &info->fix;
>>  	unsigned int yres = info->var.yres;
>> +	int xoffset = var->xoffset;
>> +	int yoffset = var->yoffset;
>>  	int err = 0;
>>  
>>  	if (var->yoffset > 0) {
>> @@ -873,7 +875,8 @@ 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->xoffset + info->var.xres > info->var.xres_virtual ||
>> +	    xoffset < 0 || yoffset < 0)
>>     
>
> Well negative xoffset/yoffset don't really exist so what you're
> essentially checking is whether offset+res overflows. Your check will
> not catch all overflows though. xres/yres would have to be huge
> (> 2^31) to cause such overflows though so your check should catch all
> cases that can happen in practice. However I think it would be better
> to make the overflow check clearer (eg. 'offset + res < res').
>
>   


------------------------------------------------------------------------------

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

[Ville Syrjälä]

On Tue, Jun 30, 2009 at 11:25:52AM +0800, Kai Jiang wrote:
> 
> xres,yres,xres_virtual,yres_virtual will be set in the display driver, they have actual value which match the screen(will not very huge).
> 
> And here, these lines are not checking the overflowing, but checking whether the virtual space display is beyond the real screen.
> 
> Take x boundary for example:(the screen x and virtual x relationship should be:)
> xres+xoffset<xres_vritual && xoffset>0
> This insure that, in the real screen, there are virtual picture display in the screen. When we move the virtual picture to the left, the xoffset will be smaller and smaller
> to indicate the virtual picture is moving to the left. Finally, the xoffset will be zero when the screen left boundary and virtual picture left boundary overlap. When virtual display is still moving to the left, the xoffset will be negative. And what should be displayed in the gap between screen left boundary and virtual space left boundary?

If you're storing negative values into xoffset/yoffset your application
is buggy. Just fix it.

> So here we have to check the whether the x/yoffset is smaller than zero. If the offset is smaller than zero, in the driver, we should not move the virtual screen any more.

Checking for overflow will catch you buggy application's negative
values too.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

------------------------------------------------------------------------------

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

[Florian Tobias Schandinat]

Ville Syrjälä schrieb:
>> So here we have to check the whether the x/yoffset is smaller than zero. If the offset is smaller than zero, in the driver, we should not move the virtual screen any more.
> 
> Checking for overflow will catch you buggy application's negative
> values too.

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


Greetings,

Florian Tobias Schandinat

------------------------------------------------------------------------------

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

[Kai Jiang]

Florian Tobias Schandinat wrote:
> Ville Syrjälä schrieb:
>>> So here we have to check the whether the x/yoffset is smaller than 
>>> zero. If the offset is smaller than zero, in the driver, we should 
>>> not move the virtual screen any more.
>>
>> Checking for overflow will catch you buggy application's negative
>> values too.
>
> 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?
Thanks!

Best Regards,

Kai Jiang



------------------------------------------------------------------------------

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

[Florian Tobias Schandinat]

Kai Jiang schrieb:
> Florian Tobias Schandinat wrote:
>> Ville Syrjälä schrieb:
>>>> So here we have to check the whether the x/yoffset is smaller than 
>>>> zero. If the offset is smaller than zero, in the driver, we should 
>>>> not move the virtual screen any more.
>>>
>>> Checking for overflow will catch you buggy application's negative
>>> values too.
>>
>> 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?

A small program to illustrate it:

#include <stdio.h>
int main()
{
         unsigned int    a = -1;
         printf( "%X\n%X\n", a, a+1 );
         return 0;
}

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.

Hope this helps.


Greetings,

Florian Tobias Schandinat

------------------------------------------------------------------------------

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

[Kai Jiang]

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

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

[Florian Tobias Schandinat]

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

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

[Kai Jiang]

Florian Tobias Schandinat,

I think your solution is better. It seems much simple in your way:)

Best Regards,

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


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

[PATCH] fb: fix fb_pan_display range check

[Florian Tobias Schandinat]

fb: fix fb_pan_display range check

This patch fixes the range check for panning. The current code fails to detect
some invalid values (very high ones that can occur if an app tries to move
further up/left than 0,0) as the check uses the unknown values for calculation
so that an overflow can occur. To fix this it is sufficient to move the
calculation to the right side to use only trusted values.
Kai Jiang detected this problem and proposed an initial patch.

Signed-off-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
---
 drivers/video/fbmem.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a85c818..346f257 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -871,8 +871,8 @@ fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var)
 		err = -EINVAL;
 
 	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 > info->var.yres_virtual - yres ||
+	    var->xoffset > info->var.xres_virtual - info->var.xres)
 		return -EINVAL;
 
 	if ((err = info->fbops->fb_pan_display(var, info)))
-- 
1.6.3.2

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

[Kai Jiang]

We can't suppose that all the user operating this x/yoffset will check 
it. If the user application transfer a negative x/yoffset into the fb 
driver, I suppose fb driver should do operation correctly. So here, I 
think checking x/yoffset to insure the virtual space not moving outside 
real screen is necessary.
> On Tue, Jun 30, 2009 at 11:25:52AM +0800, Kai Jiang wrote:
>   
>> xres,yres,xres_virtual,yres_virtual will be set in the display driver, they have actual value which match the screen(will not very huge).
>>
>> And here, these lines are not checking the overflowing, but checking whether the virtual space display is beyond the real screen.
>>
>> Take x boundary for example:(the screen x and virtual x relationship should be:)
>> xres+xoffset<xres_vritual && xoffset>0
>> This insure that, in the real screen, there are virtual picture display in the screen. When we move the virtual picture to the left, the xoffset will be smaller and smaller
>> to indicate the virtual picture is moving to the left. Finally, the xoffset will be zero when the screen left boundary and virtual picture left boundary overlap. When virtual display is still moving to the left, the xoffset will be negative. And what should be displayed in the gap between screen left boundary and virtual space left boundary?
>>     
>
> If you're storing negative values into xoffset/yoffset your application
> is buggy. Just fix it.
>
>   
>> So here we have to check the whether the x/yoffset is smaller than zero. If the offset is smaller than zero, in the driver, we should not move the virtual screen any more.
>>     
>
> Checking for overflow will catch you buggy application's negative
> values too.
>
>   


------------------------------------------------------------------------------