[RFC] [patch] fbcmap: integer overflow bug

[RFC] [patch] fbcmap: integer overflow bug

[Dan Carpenter]

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?

diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index f53b9f1..6dc5817 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -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)

Re: [RFC] [patch] fbcmap: integer overflow bug

[Andrew Morton]

On Wed, 27 Oct 2010 11:37:16 +0200
Dan Carpenter <error27@gmail.com> 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.
> 
> 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:.

> diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
> index f53b9f1..6dc5817 100644
> --- a/drivers/video/fbcmap.c
> +++ b/drivers/video/fbcmap.c
> @@ -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)

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

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

[Dan Carpenter]

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

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/video/fbcmap.c b/drivers/video/fbcmap.c
index f53b9f1..a79b976 100644
--- a/drivers/video/fbcmap.c
+++ b/drivers/video/fbcmap.c
@@ -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;
 }
 
 /**

[patch 2/2] fbcmap: integer overflow bug

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

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@gmail.com>

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

Re: [patch 2/2] fbcmap: integer overflow bug

[Paul Mundt]

On Sat, Nov 13, 2010 at 01:07:18PM +0300, Dan Carpenter wrote:
> @@ -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) ||

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.

Re: [patch 2/2] fbcmap: integer overflow bug

[Geert Uytterhoeven]

On Mon, Nov 15, 2010 at 05:48, Paul Mundt <lethal@linux-sh.org> wrote:
> On Sat, Nov 13, 2010 at 01:07:18PM +0300, Dan Carpenter wrote:
>> @@ -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?

>> +             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.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [patch 2/2] fbcmap: integer overflow bug

[Dan Carpenter]

On Mon, Nov 15, 2010 at 07:56:05AM +0100, Geert Uytterhoeven wrote:
> On Mon, Nov 15, 2010 at 05:48, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Sat, Nov 13, 2010 at 01:07:18PM +0300, Dan Carpenter wrote:
> >> @@ -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

Re: [patch 2/2] fbcmap: integer overflow bug

[walter harms]

Am 15.11.2010 08:20, schrieb Dan Carpenter:
> On Mon, Nov 15, 2010 at 07:56:05AM +0100, Geert Uytterhoeven wrote:
>> On Mon, Nov 15, 2010 at 05:48, Paul Mundt <lethal@linux-sh.org> wrote:
>>> On Sat, Nov 13, 2010 at 01:07:18PM +0300, Dan Carpenter wrote:
>>>> @@ -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.
> 

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

Re: [patch 2/2] fbcmap: integer overflow bug

[Dan Carpenter]

On Mon, Nov 15, 2010 at 09:01:14AM +0100, walter harms wrote:
> 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.

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

[patch 2/2 v2] fbcmap: integer overflow bug

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

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@gmail.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) ||

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

[Paul Mundt]

On Sat, Nov 13, 2010 at 01:06:38PM +0300, Dan Carpenter wrote:
> checkpatch.pl and Andrew Morton both complained about the indenting in
> fb_alloc_cmap()

On Tue, Nov 16, 2010 at 12:11:02PM +0300, Dan Carpenter wrote:
> 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.

Both applied, thanks.