linux-kernel - Re: [Linux-fbdev-devel] [PATCH] fbdev: Wrong test on unsigned in fb_set_user

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <20091021204722.20ccb7a9.krzysztof.h1@poczta.fm>
Date:	Wed, 21 Oct 2009 20:47:22 +0200
From:	Krzysztof Helt <krzysztof.h1@...zta.fm>
To:	Roel Kluin <roel.kluin@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fbdev-devel@...ts.sourceforge.net,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [Linux-fbdev-devel] [PATCH] fbdev: Wrong test on unsigned in
 fb_set_user_cmap()?

On Wed, 21 Oct 2009 17:10:01 +0200
Roel Kluin <roel.kluin@...il.com> 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 <roel.kluin@...il.com>
> ---
> >> 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 <krzysztof.h1@...pl>


----------------------------------------------------------------------
Zobacz najlepsze walki Adamka.
Ogladaj >>> http://link.interia.pl/f23e8

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/