* [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper() @ 2018-08-31 8:09 ` Dan Carpenter 2018-10-08 10:49 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 3+ messages in thread From: Dan Carpenter @ 2018-08-31 8:09 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz, Peter Malone Cc: Mathieu Malaterre, linux-fbdev, kernel-janitors, dri-devel, Philippe Ombredanne 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++) { ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper() 2018-08-31 8:09 ` [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper() Dan Carpenter @ 2018-10-08 10:49 ` Bartlomiej Zolnierkiewicz 2018-10-17 14:05 ` Dan Carpenter 0 siblings, 1 reply; 3+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2018-10-08 10:49 UTC (permalink / raw) To: Dan Carpenter Cc: linux-fbdev, Mathieu Malaterre, kernel-janitors, dri-devel, Philippe Ombredanne, Peter Malone 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper() 2018-10-08 10:49 ` Bartlomiej Zolnierkiewicz @ 2018-10-17 14:05 ` Dan Carpenter 0 siblings, 0 replies; 3+ messages in thread From: Dan Carpenter @ 2018-10-17 14:05 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz Cc: linux-fbdev, Mathieu Malaterre, kernel-janitors, dri-devel, Philippe Ombredanne, Peter Malone 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-17 14:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20180831081000epcas1p34f5574107a61b0ef8fe700b2956c4196@epcas1p3.samsung.com>
2018-08-31 8:09 ` [PATCH 2/2] fbdev: sbuslib: integer overflow in sbusfb_ioctl_helper() Dan Carpenter
2018-10-08 10:49 ` Bartlomiej Zolnierkiewicz
2018-10-17 14:05 ` Dan Carpenter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox