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