* [PATCH] fbdev: Wrong test on unsigned in fb_set_user_cmap()?
@ 2009-10-20 19:30 Roel Kluin
2009-11-03 22:59 ` Andrew Morton
0 siblings, 1 reply; 3+ messages in thread
From: Roel Kluin @ 2009-10-20 19:30 UTC (permalink / raw)
To: Andrew Morton, linux-fbdev-devel, LKML
struct fb_cmap_user member start is unsigned.
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Is this required?
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;
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] fbdev: Wrong test on unsigned in fb_set_user_cmap()?
@ 2009-10-21 6:50 krzysztof.h1
0 siblings, 0 replies; 3+ messages in thread
From: krzysztof.h1 @ 2009-10-21 6:50 UTC (permalink / raw)
To: Roel Kluin; +Cc: Andrew Morton, linux-fbdev-devel, LKML
"Roel Kluin" <roel.kluin@gmail.com> pisze:
> struct fb_cmap_user member start is unsigned.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> 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
> Linux-fbdev-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel
>
>
----------------------------------------------------------------------
Zobacz najwiekszy samolot na swiecie!
Kliknij >>> http://link.interia.pl/f238f
------------------------------------------------------------------------------
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] fbdev: Wrong test on unsigned in fb_set_user_cmap()?
2009-10-20 19:30 [PATCH] fbdev: Wrong test on unsigned in fb_set_user_cmap()? Roel Kluin
@ 2009-11-03 22:59 ` Andrew Morton
0 siblings, 0 replies; 3+ messages in thread
From: Andrew Morton @ 2009-11-03 22:59 UTC (permalink / raw)
To: Roel Kluin; +Cc: linux-fbdev-devel, LKML
On Tue, 20 Oct 2009 21:30:38 +0200
Roel Kluin <roel.kluin@gmail.com> wrote:
> struct fb_cmap_user member start is unsigned.
>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Is this required?
>
> 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;
I think the test _is_ needed, as `start' is passed in direct from
userspace, via do_fb_ioctl():FBIOPUTCMAP.
However fb_set_user_cmap() calls fb_set_cmap() which also checks
`start', only this time it does it correctly.
So overall the code is OK and the check which you've identified
is unneeded.
In fact, all of
if (cmap->start < 0 || (!info->fbops->fb_setcolreg &&
!info->fbops->fb_setcmap)) {
rc = -EINVAL;
goto out1;
}
can be removed.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2009-11-03 22:59 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-20 19:30 [PATCH] fbdev: Wrong test on unsigned in fb_set_user_cmap()? Roel Kluin
2009-11-03 22:59 ` Andrew Morton
-- strict thread matches above, loose matches on Subject: below --
2009-10-21 6:50 krzysztof.h1
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).