Menu
▾
▴
linux-fbdev-devel
|
From: Kai J. <b1...@fr...> - 2009-06-29 03:45:58
Attachments:
fb_pan_display-check-offset.patch
|
|
From: Ville S. <sy...@sc...> - 2009-06-29 10:39:46
|
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...@fr...>
> 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...@fr...>
> ---
> 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ä
sy...@sc...
http://www.sci.fi/~syrjala/
|
|
From: Kai J. <b1...@fr...> - 2009-06-30 03:22:25
|
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...@fr...>
>> 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...@fr...>
>> ---
>> 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').
>
>
|
|
From: Ville S. <sy...@sc...> - 2009-07-03 15:30:44
|
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ä sy...@sc... http://www.sci.fi/~syrjala/ |
|
From: Kai J. <b1...@fr...> - 2009-07-06 02:54:58
|
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. > > |
|
From: Kai J. <b1...@fr...> - 2009-07-07 02:43:36
|
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
|
|
From: Florian T. S. <Flo...@gm...> - 2009-07-07 04:06:15
|
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
|
|
From: Kai J. <b1...@fr...> - 2009-07-10 08:42:28
|
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;
> ......
> }
|
|
From: Florian T. S. <Flo...@gm...> - 2009-08-03 18:57:33
|
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 <Flo...@gm...> --- 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 |
|
From: Florian T. S. <Flo...@gm...> - 2009-07-03 16:17:18
|
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 |
|
From: Kai J. <b1...@fr...> - 2009-07-06 03:01:01
|
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 |
|
From: Florian T. S. <Flo...@gm...> - 2009-07-06 14:17:49
|
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
|
×
Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.
Thanks for helping keep SourceForge clean.
X