| Message ID | 20180516140026.GA19340@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Dan Carpenter (2018-05-16 15:00:26) > There is a comment here which says that DIV_ROUND_UP() and that's where > the problem comes from. Say you pick: > > args->bpp = UINT_MAX - 7; > args->width = 4; > args->height = 1; > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > because of how we picked args->width that means cpp < UINT_MAX / 4. > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > removed the check for !cpp because it's not possible after this change. > I also changed all the 0xffffffffU references to U32_MAX. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> I agree with Daniel that the !cpp check after DIV_ROUND_UP was sufficient to guard the current code, but switching to a more idiomatic style of overflow checking has its benefits too. Plus I like having U32_MAX to compare the type ranges against. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, May 16, 2018 at 03:26:07PM +0100, Chris Wilson wrote: > Quoting Dan Carpenter (2018-05-16 15:00:26) > > There is a comment here which says that DIV_ROUND_UP() and that's where > > the problem comes from. Say you pick: > > > > args->bpp = UINT_MAX - 7; > > args->width = 4; > > args->height = 1; > > > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > > because of how we picked args->width that means cpp < UINT_MAX / 4. > > > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > > removed the check for !cpp because it's not possible after this change. > > I also changed all the 0xffffffffU references to U32_MAX. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > I agree with Daniel that the !cpp check after DIV_ROUND_UP was > sufficient to guard the current code, but switching to a more idiomatic > style of overflow checking has its benefits too. Plus I like having > U32_MAX to compare the type ranges against. > I'm not totally sure what it means to say that the integer overflow is sufficient. The overflow check is definitely buggy but if you mean that it isn't exploitable then you're probably right. Anyway, let's say you use use the values I provided in my changelog. Then I believe we can reach vgem_gem_dumb_create(): static int vgem_gem_dumb_create(struct drm_file *file, struct drm_device *dev, 205 struct drm_mode_create_dumb *args) 206 { 207 struct drm_gem_object *gem_object; 208 u64 pitch, size; 209 210 pitch = args->width * DIV_ROUND_UP(args->bpp, 8); ^^^^^^^^^^^^^^^^^^^^^^^^^^ This is now: pitch = 4 * (U32_MAX / 8); I would get a static checker warning here because of the integer overflow in DIV_ROUND_UP(). I'll push that check soon. 211 size = args->height * pitch; This is now: size = 1 * U32_MAX / 2; 212 if (size == 0) 213 return -EINVAL; 214 215 gem_object = vgem_gem_create(dev, file, &args->handle, size); Then this fails because we can't allocate U32_MAX / 2 bytes. There are some other potential numbers we could try instead of the ones I gave. On 32bit arches __vgem_gem_create() has integer overflows if size is over U32_MAX - PAGE_SIZE so it gets a bit complicated... 216 if (IS_ERR(gem_object)) 217 return PTR_ERR(gem_object); 218 219 args->size = gem_object->size; 220 args->pitch = pitch; 221 222 DRM_DEBUG_DRIVER("Created object of size %lld\n", size); 223 224 return 0; 225 } regards, dan carpenter
Quoting Dan Carpenter (2018-05-16 15:52:57) > On Wed, May 16, 2018 at 03:26:07PM +0100, Chris Wilson wrote: > > Quoting Dan Carpenter (2018-05-16 15:00:26) > > > There is a comment here which says that DIV_ROUND_UP() and that's where > > > the problem comes from. Say you pick: > > > > > > args->bpp = UINT_MAX - 7; > > > args->width = 4; > > > args->height = 1; > > > > > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > > > because of how we picked args->width that means cpp < UINT_MAX / 4. > > > > > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > > > removed the check for !cpp because it's not possible after this change. > > > I also changed all the 0xffffffffU references to U32_MAX. > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > I agree with Daniel that the !cpp check after DIV_ROUND_UP was > > sufficient to guard the current code, but switching to a more idiomatic > > style of overflow checking has its benefits too. Plus I like having > > U32_MAX to compare the type ranges against. > > > > I'm not totally sure what it means to say that the integer overflow is > sufficient. The overflow check is definitely buggy but if you mean that > it isn't exploitable then you're probably right. Anyway, let's say you > use use the values I provided in my changelog. Then I believe we can > reach vgem_gem_dumb_create(): But we are talking about cpp = (U32_MAX + 8) / 8 which is 0. So the !cpp does catch the overflow. Or am I completely off in unsigned wraparound? -Chris
On Wed, May 16, 2018 at 03:56:55PM +0100, Chris Wilson wrote: > Quoting Dan Carpenter (2018-05-16 15:52:57) > > On Wed, May 16, 2018 at 03:26:07PM +0100, Chris Wilson wrote: > > > Quoting Dan Carpenter (2018-05-16 15:00:26) > > > > There is a comment here which says that DIV_ROUND_UP() and that's where > > > > the problem comes from. Say you pick: > > > > > > > > args->bpp = UINT_MAX - 7; > > > > args->width = 4; > > > > args->height = 1; > > > > > > > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > > > > because of how we picked args->width that means cpp < UINT_MAX / 4. > > > > > > > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > > > > removed the check for !cpp because it's not possible after this change. > > > > I also changed all the 0xffffffffU references to U32_MAX. > > > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > I agree with Daniel that the !cpp check after DIV_ROUND_UP was > > > sufficient to guard the current code, but switching to a more idiomatic > > > style of overflow checking has its benefits too. Plus I like having > > > U32_MAX to compare the type ranges against. > > > > > > > I'm not totally sure what it means to say that the integer overflow is > > sufficient. The overflow check is definitely buggy but if you mean that > > it isn't exploitable then you're probably right. Anyway, let's say you > > use use the values I provided in my changelog. Then I believe we can > > reach vgem_gem_dumb_create(): > > But we are talking about > > cpp = (U32_MAX + 8) / 8 > No, no. That's not the problem. The problem is the - 1 subtraction. I explained poorly. The issue is when "bpp" is U32_MAX - 7. The define looks like: #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) So "n" is (U32_MAX - 7) and "d" is 8. "(n) + (d)" wraps to zero. We do the "- 1" subtraction so that gives us U32_MAX. And finally we divide by "d" to get "U32_MAX / 8". regards, dan carpenter
Quoting Dan Carpenter (2018-05-16 16:15:54) > On Wed, May 16, 2018 at 03:56:55PM +0100, Chris Wilson wrote: > > Quoting Dan Carpenter (2018-05-16 15:52:57) > > > On Wed, May 16, 2018 at 03:26:07PM +0100, Chris Wilson wrote: > > > > Quoting Dan Carpenter (2018-05-16 15:00:26) > > > > > There is a comment here which says that DIV_ROUND_UP() and that's where > > > > > the problem comes from. Say you pick: > > > > > > > > > > args->bpp = UINT_MAX - 7; > > > > > args->width = 4; > > > > > args->height = 1; > > > > > > > > > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > > > > > because of how we picked args->width that means cpp < UINT_MAX / 4. > > > > > > > > > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > > > > > removed the check for !cpp because it's not possible after this change. > > > > > I also changed all the 0xffffffffU references to U32_MAX. > > > > > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > I agree with Daniel that the !cpp check after DIV_ROUND_UP was > > > > sufficient to guard the current code, but switching to a more idiomatic > > > > style of overflow checking has its benefits too. Plus I like having > > > > U32_MAX to compare the type ranges against. > > > > > > > > > > I'm not totally sure what it means to say that the integer overflow is > > > sufficient. The overflow check is definitely buggy but if you mean that > > > it isn't exploitable then you're probably right. Anyway, let's say you > > > use use the values I provided in my changelog. Then I believe we can > > > reach vgem_gem_dumb_create(): > > > > But we are talking about > > > > cpp = (U32_MAX + 8) / 8 > > > > No, no. That's not the problem. The problem is the - 1 subtraction. I > explained poorly. The issue is when "bpp" is U32_MAX - 7. The define > looks like: > > #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)) > > So "n" is (U32_MAX - 7) and "d" is 8. "(n) + (d)" wraps to zero. We do > the "- 1" subtraction so that gives us U32_MAX. And finally we divide > by "d" to get "U32_MAX / 8". D'oh. Thanks for spelling it out. -Chris
Btw, I've looked at this some more and I'm 99% sure there is no way to exploit it. The "if (PAGE_ALIGN(size) == 0)" prevents the integer overflow in __vgem_gem_create() that I was worried about. regards, dan carpenter
On Wed, May 16, 2018 at 05:00:26PM +0300, Dan Carpenter wrote: > There is a comment here which says that DIV_ROUND_UP() and that's where > the problem comes from. Say you pick: > > args->bpp = UINT_MAX - 7; > args->width = 4; > args->height = 1; > > The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and > because of how we picked args->width that means cpp < UINT_MAX / 4. > > I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I > removed the check for !cpp because it's not possible after this change. > I also changed all the 0xffffffffU references to U32_MAX. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: additional cleanups Thanks a lot for respinning, applied to drm-misc-fixes. -Daniel > > diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c > index 39ac15ce4702..9e2ae02f31e0 100644 > --- a/drivers/gpu/drm/drm_dumb_buffers.c > +++ b/drivers/gpu/drm/drm_dumb_buffers.c > @@ -65,12 +65,13 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, > return -EINVAL; > > /* overflow checks for 32bit size calculations */ > - /* NOTE: DIV_ROUND_UP() can overflow */ > + if (args->bpp > U32_MAX - 8) > + return -EINVAL; > cpp = DIV_ROUND_UP(args->bpp, 8); > - if (!cpp || cpp > 0xffffffffU / args->width) > + if (cpp > U32_MAX / args->width) > return -EINVAL; > stride = cpp * args->width; > - if (args->height > 0xffffffffU / stride) > + if (args->height > U32_MAX / stride) > return -EINVAL; > > /* test for wrap-around */ > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_dumb_buffers.c b/drivers/gpu/drm/drm_dumb_buffers.c index 39ac15ce4702..9e2ae02f31e0 100644 --- a/drivers/gpu/drm/drm_dumb_buffers.c +++ b/drivers/gpu/drm/drm_dumb_buffers.c @@ -65,12 +65,13 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev, return -EINVAL; /* overflow checks for 32bit size calculations */ - /* NOTE: DIV_ROUND_UP() can overflow */ + if (args->bpp > U32_MAX - 8) + return -EINVAL; cpp = DIV_ROUND_UP(args->bpp, 8); - if (!cpp || cpp > 0xffffffffU / args->width) + if (cpp > U32_MAX / args->width) return -EINVAL; stride = cpp * args->width; - if (args->height > 0xffffffffU / stride) + if (args->height > U32_MAX / stride) return -EINVAL; /* test for wrap-around */
There is a comment here which says that DIV_ROUND_UP() and that's where the problem comes from. Say you pick: args->bpp = UINT_MAX - 7; args->width = 4; args->height = 1; The integer overflow in DIV_ROUND_UP() means "cpp" is UINT_MAX / 8 and because of how we picked args->width that means cpp < UINT_MAX / 4. I've fixed it by preventing the integer overflow in DIV_ROUND_UP(). I removed the check for !cpp because it's not possible after this change. I also changed all the 0xffffffffU references to U32_MAX. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: additional cleanups