[PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper()

[PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper()

[Dan Carpenter]

The "index + count" addition can overflow.  Both come directly from the
user.  This bug leads to an information leak.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
Btw, commit 250c6c49e3b6 ("fbdev: Fixing arbitrary kernel leak in case
FBIOGETCMAP_SPARC in sbusfb_ioctl_helper().") doesn't do anything so
far as I can see.  The "cmap->len" variable is type u32, so the
comparison was already unsigned in the original code because of type
promotion.  But the commit was also harmless and nice cleanup.

diff --git a/drivers/video/fbdev/sbuslib.c b/drivers/video/fbdev/sbuslib.c
index 90c51330969c..01a7110e61a7 100644
--- a/drivers/video/fbdev/sbuslib.c
+++ b/drivers/video/fbdev/sbuslib.c
@@ -171,7 +171,7 @@ int sbusfb_ioctl_helper(unsigned long cmd, unsigned long arg,
 		    get_user(ublue, &c->blue))
 			return -EFAULT;
 
-		if (index + count > cmap->len)
+		if (index > cmap->len || count > cmap->len - index)
 			return -EINVAL;
 
 		for (i = 0; i < count; i++) {

Re: [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper()

[Bartlomiej Zolnierkiewicz]

On 08/31/2018 10:09 AM, Dan Carpenter wrote:
> The "index + count" addition can overflow.  Both come directly from the
> user.  This bug leads to an information leak.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Patch queued for 4.20, thanks.

> ---
> Btw, commit 250c6c49e3b6 ("fbdev: Fixing arbitrary kernel leak in case
> FBIOGETCMAP_SPARC in sbusfb_ioctl_helper().") doesn't do anything so
> far as I can see.  The "cmap->len" variable is type u32, so the
> comparison was already unsigned in the original code because of type
> promotion.  But the commit was also harmless and nice cleanup.

Both 'index' and 'count' are controlled by user so they could be set to
i.e. -100 and 100 accordingly. Such arguments would pass the 'if' test
(because '+' happens before  type promotion) but still result in leaking
kernel memory (inside 'for' loop).

> diff --git a/drivers/video/fbdev/sbuslib.c b/drivers/video/fbdev/sbuslib.c
> index 90c51330969c..01a7110e61a7 100644
> --- a/drivers/video/fbdev/sbuslib.c
> +++ b/drivers/video/fbdev/sbuslib.c
> @@ -171,7 +171,7 @@ int sbusfb_ioctl_helper(unsigned long cmd, unsigned long arg,
>  		    get_user(ublue, &c->blue))
>  			return -EFAULT;
>  
> -		if (index + count > cmap->len)
> +		if (index > cmap->len || count > cmap->len - index)
>  			return -EINVAL;
>  
>  		for (i = 0; i < count; i++) {

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

Re: [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper()

[Dan Carpenter]

On Mon, Oct 08, 2018 at 12:49:07PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> On 08/31/2018 10:09 AM, Dan Carpenter wrote:
> > The "index + count" addition can overflow.  Both come directly from the
> > user.  This bug leads to an information leak.
> > 
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Patch queued for 4.20, thanks.
> 
> > ---
> > Btw, commit 250c6c49e3b6 ("fbdev: Fixing arbitrary kernel leak in case
> > FBIOGETCMAP_SPARC in sbusfb_ioctl_helper().") doesn't do anything so
> > far as I can see.  The "cmap->len" variable is type u32, so the
> > comparison was already unsigned in the original code because of type
> > promotion.  But the commit was also harmless and nice cleanup.
> 
> Both 'index' and 'count' are controlled by user so they could be set to
> i.e. -100 and 100 accordingly. Such arguments would pass the 'if' test
> (because '+' happens before  type promotion) but still result in leaking
> kernel memory (inside 'for' loop).

It's still basically the same when it's unsigned.

Before:  -100 + 100 => 0
 After:  -100U + 100U => 0

The result of the math is still zero.  It's hard to know how to catch
this sort of bug...

regards,
dan carpenter