fbcmap: integer overflow bug

RFC PATCH Superseded Dan Carpenter 5 participants 11 messages Oct 27, 2010 – Nov 18, 2010

Subsystems (2)

Kernel janitors Framebuffer layer

lore

Share & tools

lore

Dan Carpenter

author

2010-10-27 09:37 UTC #0 lore raw

RFC PATCH 1/1 fbcmap: integer overflow bug

There is an integer overflow bug in the FBIOPUTCMAP ioctl if cmap->len * 2 overflows.

It's harmless, except that it messes up the cmap until someone types reset. It could have been caught by checking the return from fb_copy_cmap().

Or it could have been caught by limiting the size of the cmaps to one page. The cmaps are allocated with GFP_ATOMIC and it makes sense to limit them.

Different drivers use different sizes of cmaps. There are about 150 drivers. I've checked a bunch (50) of them and the larges cmap.len I've found is gxt4500 which maxes out at 1024 so PAGE_SIZE is about twice that length. For some of the 50 I wasn't sure on the limit.

Is PAGE_SIZE a reasonable limit? Does anyone know or do I have to audit all 150 drivers?

drivers/video/fbcmap.c +6 -1

index f53b9f1..6dc5817 100644
@@ -110,7 +110,9 @@ 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);
+    if (fb_copy_cmap(fb_default_cmap(len), cmap))
+	goto fail;
+
     return 0;
 
 fail:

@@ -250,6 +252,9 @@ 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 (cmap->len > PAGE_SIZE || cmap->len * sizeof(u16) > PAGE_SIZE)
+		return -EINVAL;
+
 	memset(&umap, 0, sizeof(struct fb_cmap));
 	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
 	if (rc)

Andrew Morton

2010-11-05 20:40 UTC #1 lore raw

Quoted 11 lines · On Wed, 27 Oct 2010 11:37:16 +0200 Dan Carpenter <[email protected]> wrote:

There is an integer overflow bug in the FBIOPUTCMAP ioctl if cmap->len * 2 overflows.
It's harmless, except that it messes up the cmap until someone types reset. It could have been caught by checking the return from fb_copy_cmap().
Or it could have been caught by limiting the size of the cmaps to one page. The cmaps are allocated with GFP_ATOMIC and it makes sense to limit them.

Quoted 7 lines

Different drivers use different sizes of cmaps. There are about 150 drivers. I've checked a bunch (50) of them and the larges cmap.len I've found is gxt4500 which maxes out at 1024 so PAGE_SIZE is about twice that length. For some of the 50 I wasn't sure on the limit.
Is PAGE_SIZE a reasonable limit? Does anyone know or do I have to audit all 150 drivers?

No signed-off-by:.

Quoted 16 lines

drivers/video/fbcmap.c +3 -1

index f53b9f1..6dc5817 100644
@@ -110,7 +110,9 @@ 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);
+    if (fb_copy_cmap(fb_default_cmap(len), cmap))
+	goto fail;
+
     return 0;
 
 fail:

drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 260

Quoted 9 lines

drivers/video/fbcmap.c +3 -0

@@ -250,6 +252,9 @@ 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 (cmap->len > PAGE_SIZE || cmap->len * sizeof(u16) > PAGE_SIZE)
+		return -EINVAL;
+
 	memset(&umap, 0, sizeof(struct fb_cmap));
 	rc = fb_alloc_cmap(&umap, cmap->len, cmap->transp != NULL);
 	if (rc)

A suitable way to fix this would be to detect the overflow only, and then just throw the passed-in length at kmalloc(), and let kmalloc() decide whether it is sane or not.

To do that, one would need to implement a new fb_alloc_cmap_not_stupid() which takes a gfp_t, and pass in GFP_KERNEL.

Feel free to sanitise the fb_alloc_cmap() indenting in [patch 1/2] as well ;)

Dan Carpenter

author

2010-11-13 10:06 UTC #2 lore raw

PATCH 1/2 fbcmap: cleanup white space in fb_alloc_cmap()

checkpatch.pl and Andrew Morton both complained about the indenting in fb_alloc_cmap()

Signed-off-by Dan Carpenter <[email protected]>

drivers/video/fbcmap.c +30 ~~-24~~

index f53b9f1..a79b976 100644
@@ -90,32 +90,38 @@ static const struct fb_cmap default_16_colors = {
 
 int fb_alloc_cmap(struct fb_cmap *cmap, int len, int transp)
 {
-    int size = len*sizeof(u16);
-
-    if (cmap->len != len) {
-	fb_dealloc_cmap(cmap);
-	if (!len)
-	    return 0;
-	if (!(cmap->red = kmalloc(size, GFP_ATOMIC)))
-	    goto fail;
-	if (!(cmap->green = kmalloc(size, GFP_ATOMIC)))
-	    goto fail;
-	if (!(cmap->blue = kmalloc(size, GFP_ATOMIC)))
-	    goto fail;
-	if (transp) {
-	    if (!(cmap->transp = kmalloc(size, GFP_ATOMIC)))
-		goto fail;
-	} else
-	    cmap->transp = NULL;
-    }
-    cmap->start = 0;
-    cmap->len = len;
-    fb_copy_cmap(fb_default_cmap(len), cmap);
-    return 0;
+	int size = len * sizeof(u16);
+
+	if (cmap->len != len) {
+		fb_dealloc_cmap(cmap);
+		if (!len)
+			return 0;
+
+		cmap->red = kmalloc(size, GFP_ATOMIC);
+		if (!cmap->red)
+			goto fail;
+		cmap->green = kmalloc(size, GFP_ATOMIC);
+		if (!cmap->green)
+			goto fail;
+		cmap->blue = kmalloc(size, GFP_ATOMIC);
+		if (!cmap->blue)
+			goto fail;
+		if (transp) {
+			cmap->transp = kmalloc(size, GFP_ATOMIC);
+			if (!cmap->transp)
+				goto fail;
+		} else {
+			cmap->transp = NULL;
+		}
+	}
+	cmap->start = 0;
+	cmap->len = len;
+	fb_copy_cmap(fb_default_cmap(len), cmap);
+	return 0;
 
 fail:
-    fb_dealloc_cmap(cmap);
-    return -ENOMEM;
+	fb_dealloc_cmap(cmap);
+	return -ENOMEM;
 }
 
 /**

Dan Carpenter

author

2010-11-13 10:07 UTC #3 lore raw

PATCH 2/2 fbcmap: integer overflow bug

In reply to Andrew Morton

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 <[email protected]>

include/linux/fb.h +1 -0

index 7fca3dc..d1631d3 100644
@@ -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);

drivers/video/fbcmap.c +20 -8

index a79b976..e6a01bf 100644
@@ -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 (cmap->len * 2 > INT_MAX)
+		return -EINVAL;
+
 	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) ||

Paul Mundt

2010-11-15 04:48 UTC #4 lore raw

Quoted 4 lines · Dan Carpenter

drivers/video/fbcmap.c +0 -0

@@ -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;

drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 275

Quoted 10 lines

drivers/video/fbcmap.c +5 -1

+	if (cmap->len * 2 > INT_MAX)
+		return -EINVAL;
+
 	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) ||

This looks reasonable, but it probably makes more sense to use -E2BIG for the overflow case (as other cases are doing already), and also just to check size directly rather than open-coding the * 2.

Geert Uytterhoeven

2010-11-15 06:56 UTC #5 lore raw

Quoted 6 lines · Paul Mundt

drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 267
Quoted 5 lines · Dan Carpenter
drivers/video/fbcmap.c +1 -0
@@ -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 (cmap->len * 2 > INT_MAX)

Isn't that another integer overflow? I.e. should be "if (cmap->len > INT_MAX / sizeof(u16))" instead?

Quoted 10 lines

Quoted 9 lines

drivers/video/fbcmap.c +4 -1

+             return -EINVAL;
+
       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) ||

Quoted 3 lines

This looks reasonable, but it probably makes more sense to use -E2BIG for the overflow case (as other cases are doing already), and also just to check size directly rather than open-coding the * 2.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds

Dan Carpenter

author

2010-11-15 07:20 UTC #6 lore raw

Quoted 11 lines · Geert Uytterhoeven

Quoted 6 lines · Paul Mundt
drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 267
Quoted 5 lines · Dan Carpenter
drivers/video/fbcmap.c +1 -0
@@ -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 (cmap->len * 2 > INT_MAX)
Isn't that another integer overflow? I.e. should be "if (cmap->len > INT_MAX / sizeof(u16))" instead?

Yeah it is. :/

I'll change it to:

	if (size < 0 || size < cmap->len)

like Paul asked.

regards,
dan carpenter

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

walter harms

2010-11-15 08:01 UTC #7 lore raw

Am 15.11.2010 08:20, schrieb Dan Carpenter:

Quoted 13 lines · Geert Uytterhoeven

Quoted 11 lines · Paul Mundt
Quoted 6 lines · Dan Carpenter
drivers/video/fbcmap.c int fb_set_user_cmap(struct fb_cmap_user *cmap, struct fb_info *info) · line 267
Quoted 5 lines
drivers/video/fbcmap.c +1 -0
@@ -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 (cmap->len * 2 > INT_MAX)
Isn't that another integer overflow? I.e. should be "if (cmap->len > INT_MAX / sizeof(u16))" instead?

Quoted 6 lines

Yeah it is. :/
I'll change it to:
	if (size < 0 || size < cmap->len)
like Paul asked.

I do not see the rest of the code but it looks like cmap->len is size in int8_t. So the upper limit is something like INT_MAX/(sizeof(u16)*2). Perhaps we can call it a char ? is there ANY system that has a more than 256 colors in R|G|B ?

<spying for struct fb_cmap_user >
__u32 len;
__u16 __user *red;

So no need to check for <0.

hope that helps,
re,
wh

Dan Carpenter

author

2010-11-15 08:14 UTC #8 lore raw

Quoted 5 lines · walter harms

I do not see the rest of the code but it looks like cmap->len is size in int8_t. So the upper limit is something like INT_MAX/(sizeof(u16)*2). Perhaps we can call it a char ? is there ANY system that has a more than 256 colors in R|G|B ?

Yeah. There are. I had a list of some but I've deleted it.

Quoted 5 lines

<spying for struct fb_cmap_user >
__u32 len;
__u16 __user *red;
So no need to check for <0.

I test size which is an int so that's why it's needed.

regards,
dan carpenter

Dan Carpenter

author

2010-11-16 09:11 UTC #9 lore raw

PATCH v2 2/2 fbcmap: integer overflow bug

In reply to Paul Mundt

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().

Signed-off-by Dan Carpenter <[email protected]>

---
v2: Fix overflow check in fb_set_user_cmap(). Return -E2BIG on error.

include/linux/fb.h +1 -0

index 7fca3dc..d1631d3 100644
@@ -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);

drivers/video/fbcmap.c +20 -8

index a79b976..affdf3e 100644
@@ -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) ||

Paul Mundt

2010-11-18 06:00 UTC #10 lore raw

Re: [patch 1/2] fbcmap: cleanup white space in fb_alloc_cmap()

Quoted 2 lines · Dan Carpenter

checkpatch.pl and Andrew Morton both complained about the indenting in fb_alloc_cmap()

Quoted 11 lines · Dan Carpenter

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().

Quoted 7 lines

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.

Both applied, thanks.