linux-kernel - [patch 2/2 v2] fbcmap: integer overflow bug

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101116090119.GA31724@bicker>
Date:	Tue, 16 Nov 2010 12:11:02 +0300
From:	Dan Carpenter <error27@...il.com>
To:	Paul Mundt <lethal@...ux-sh.org>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-fbdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: [patch 2/2 v2] fbcmap: integer overflow bug

There is an integer overflow in fb_set_user_cmap() because cmap->len * 2
can wrap.  It's basically harmless.  Your terminal will be messed up
until you type reset.

This patch does three things to fix the bug.

First, it checks the return value of fb_copy_cmap() in fb_alloc_cmap().
That is enough to fix address the overflow.

Second it checks for the integer overflow in fb_set_user_cmap().

Lastly I wanted to cap "cmap->len" in fb_set_user_cmap() much lower
because it gets used to determine the size of allocation.  Unfortunately
no one knows what the limit should be.  Instead what this patch does
is makes the allocation happen with GFP_KERNEL instead of GFP_ATOMIC
and lets the kmalloc() decide what values of cmap->len are reasonable.
To do this, the patch introduces a function called fb_alloc_cmap_gfp()
which is like fb_alloc_cmap() except that it takes a GFP flag.

Signed-off-by: Dan Carpenter <error27@...il.com>
---
v2:  Fix overflow check in fb_set_user_cmap().  Return -E2BIG on error.

diff --git a/include/linux/fb.h b/include/linux/fb.h
index 7fca3dc..d1631d3 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -1122,6 +1122,7 @@ extern const struct fb_videomode *fb_find_best_display(const struct fb_monspecs
 
 /* drivers/video/fbcmap.c */
 extern int fb_alloc_cmap(struct fb_cmap *cmap, int len, int transp);
+extern int fb_alloc_cmap_gfp(struct fb_cmap *cmap, int len, int transp, gfp_t flags);
 extern void fb_dealloc_cmap(struct fb_cmap *cmap);
 extern int fb_copy_cmap(const struct fb_cmap *from, struct fb_cmap *to);
 extern int fb_cmap_to_user(const struct fb_cmap *from, struct fb_cmap_user *to);
diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index a79b976..affdf3e 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -88,26 +88,27 @@ static const struct fb_cmap default_16_colors = {
  *
  */
 
-int fb_alloc_cmap(struct fb_cmap *cmap, int len, int transp)
+int fb_alloc_cmap_gfp(struct fb_cmap *cmap, int len, int transp, gfp_t flags)
 {
 	int size = len * sizeof(u16);
+	int ret = -ENOMEM;
 
 	if (cmap->len != len) {
 		fb_dealloc_cmap(cmap);
 		if (!len)
 			return 0;
 
-		cmap->red = kmalloc(size, GFP_ATOMIC);
+		cmap->red = kmalloc(size, flags);
 		if (!cmap->red)
 			goto fail;
-		cmap->green = kmalloc(size, GFP_ATOMIC);
+		cmap->green = kmalloc(size, flags);
 		if (!cmap->green)
 			goto fail;
-		cmap->blue = kmalloc(size, GFP_ATOMIC);
+		cmap->blue = kmalloc(size, flags);
 		if (!cmap->blue)
 			goto fail;
 		if (transp) {
-			cmap->transp = kmalloc(size, GFP_ATOMIC);
+			cmap->transp = kmalloc(size, flags);
 			if (!cmap->transp)
 				goto fail;
 		} else {
@@ -116,12 +117,19 @@ int fb_alloc_cmap(struct fb_cmap *cmap, int len, int transp)
 	}
 	cmap->start = 0;
 	cmap->len = len;
-	fb_copy_cmap(fb_default_cmap(len), cmap);
+	ret = fb_copy_cmap(fb_default_cmap(len), cmap);
+	if (ret)
+		goto fail;
 	return 0;
 
 fail:
 	fb_dealloc_cmap(cmap);
-	return -ENOMEM;
+	return ret;
+}
+
+int fb_alloc_cmap(struct fb_cmap *cmap, int len, int transp)
+{
+	return fb_alloc_cmap_gfp(cmap, len, transp, GFP_ATOMIC);
 }
 
 /**
@@ -256,8 +264,12 @@ int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info)
 	int rc, size = cmap->len * sizeof(u16);
 	struct fb_cmap umap;
 
+	if (size < 0 || size < cmap->len)
+		return -E2BIG;
+
 	memset(&umap, 0, sizeof(struct fb_cmap));
-	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
+	rc = fb_alloc_cmap_gfp(&umap, cmap->len, cmap->transp != NULL,
+				GFP_KERNEL);
 	if (rc)
 		return rc;
 	if (copy_from_user(umap.red, cmap->red, size) ||
--
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/