| Message ID | 20180831080943.ldzpzha5suktn4ln@kili.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] fbdev: sbuslib: use checked version of put_user() | expand |
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
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
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++) {
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.