"Roel Kluin" <[email protected]> pisze:
> struct fb_cmap_user member start is unsigned.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> Is this required?
>
Drop the whole if() as exactly the same condition is checked in the fb_set_cmap() again. Anyway, the check of the cmap->start < 0 does not make any sense as the start is u32 value (most userspace addresses will be lower then 2GB on 32 bit system so the error cannot be caught by the check). I vote for removing the (cmap->start < 0) in the fb_set_cmap as well as most drivers check the start value already in driver's fb_setcolreg() function.
Best regards,
Krzysztof
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index f53b9f1..f46f05f 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -266,7 +266,7 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct
> fb_info *info)
> rc = -ENODEV;
> goto out;
> }
> - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> + if ((int)cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> !info->fbops->fb_setcmap)) {
> rc = -EINVAL;
> goto out1;
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry(R) Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
>
> ahead of the curve. Join us from November 9 - 12, 2009. Register now!
> http://p.sf.net/sfu/devconference
> _______________________________________________
> Linux-fbdev-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel
>
>
----------------------------------------------------------------------
Zobacz najwiekszy samolot na swiecie!
Kliknij >>> http://link.interia.pl/f238f
struct fb_cmap_user member start is unsigned. The same condition
is already checked in fb_set_cmap(), so this should be safe.
Signed-off-by: Roel Kluin <[email protected]>
---
>> Is this required?
>
> Drop the whole if() as exactly the same condition is checked in the
> fb_set_cmap() again.
Ok
> Anyway, the check of the cmap->start < 0 does
> not make any sense as the start is u32 value (most userspace
> addresses will be lower then 2GB on 32 bit system so the error cannot
> be caught by the check). I vote for removing the (cmap->start < 0) in
> the fb_set_cmap as well as most drivers check the start value already
> in driver's fb_setcolreg() function.
In fb_set_cmap() this is not `cmap->start' but `start' which has type
int. Therefore I think the test makes some sense there, so I left it.
>
> Best regards,
> Krzysztof
Thanks, here:
diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index f53b9f1..f46d617 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -266,11 +266,6 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
rc = -ENODEV;
goto out;
}
- if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
- !info->fbops->fb_setcmap)) {
- rc = -EINVAL;
- goto out1;
- }
rc = fb_set_cmap(&umap, info);
out1:
unlock_fb_info(info);
On Wed, 21 Oct 2009 17:10:01 +0200
Roel Kluin <[email protected]> wrote:
> struct fb_cmap_user member start is unsigned. The same condition
> is already checked in fb_set_cmap(), so this should be safe.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> >> Is this required?
> >
>
> > Anyway, the check of the cmap->start < 0 does
> > not make any sense as the start is u32 value (most userspace
> > addresses will be lower then 2GB on 32 bit system so the error cannot
> > be caught by the check). I vote for removing the (cmap->start < 0) in
> > the fb_set_cmap as well as most drivers check the start value already
> > in driver's fb_setcolreg() function.
>
> In fb_set_cmap() this is not `cmap->start' but `start' which has type
> int. Therefore I think the test makes some sense there, so I left it.
>
No, it does not. If one converts u32 value to int it must be over 2GB to be negative.
The value is not in most cases as the user space is mapped below 3GB. On the other hand,
each driver receives unsigned value of the start (converted back to unsigned). It means that the
start here should be unsigned int.
Each driver should check the value it receives. I have checked and there are only 3 drivers
which do not check properly the start value:
atafb
ep93xxfb
maxinefb
The fixes are very easy.
> >
> > Best regards,
> > Krzysztof
>
> Thanks, here:
>
> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index f53b9f1..f46d617 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -266,11 +266,6 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
> rc = -ENODEV;
> goto out;
> }
> - if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
> - !info->fbops->fb_setcmap)) {
> - rc = -EINVAL;
> - goto out1;
> - }
> rc = fb_set_cmap(&umap, info);
> out1:
> unlock_fb_info(info);
Acked-by: Krzysztof Helt <[email protected]>
----------------------------------------------------------------------
Zobacz najlepsze walki Adamka.
Ogladaj >>> http://link.interia.pl/f23e8