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