fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper()

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

Signed-off-by: Dan Carpenter <redacted>
---
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++) {

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

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

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

On Mon, Oct 08, 2018 at 12:49:07PM +0200, Bartlomiej Zolnierkiewicz wrote:

On 08/31/2018 10:09 AM, Dan Carpenter wrote:

quoted

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

Signed-off-by: Dan Carpenter <redacted>

Patch queued for 4.20, thanks.

quoted

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