drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()
Subsystems (3)
PATCH 1/1 drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()
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.
---
Btw, DIV_ROUND_UP() integer overflows have been a recurring source of bugs so I have an unreleased static checker warning specific for that. This line triggers three warnings for me on my unreleased code:
drivers/gpu/drm/drm_dumb_buffers.c:69 drm_mode_create_dumb_ioctl() warn: negative user subtract: 0-u32max - 1 drivers/gpu/drm/drm_dumb_buffers.c:69 drm_mode_create_dumb_ioctl() warn: potential integer overflow from user '(args->bpp) + (8)' drivers/gpu/drm/drm_dumb_buffers.c:69 drm_mode_create_dumb_ioctl() warn: potential integer overflow in 'DIV_ROUND_UP'
It's a pretty common idiom in the kernel to overflow and then test for it later so I'm not able to release this code because of the number of false positives that this idiom causes...
drivers/gpu/drm/drm_dumb_buffers.c +2 -1
@@ -65,7 +65,8 @@ 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 >UINT_MAX -8 ) +return -EINVAL ; cpp =DIV_ROUND_UP (args->bpp,8 );if (!cpp || cpp >0xffffffffU / args->width)return -EINVAL ;
PATCH v2 1/1 drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()
There is a comment here which says that DIV_ROUND_UP() can overflow 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.
---
v2: correct a typo in the commit message
drivers/gpu/drm/drm_dumb_buffers.c +2 -1
@@ -65,7 +65,8 @@ 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 >UINT_MAX -8 ) +return -EINVAL ; cpp =DIV_ROUND_UP (args->bpp,8 );if (!cpp || cpp >0xffffffffU / args->width)return -EINVAL ;
Quoting Dan Carpenter (2018-05-09 09:12:54)
Quoted 19 lines
There is a comment here which says that
DIV_ROUND_UP()can overflow 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" isUINT_MAX/ 8 and because of how we pickedargs->widththat means cpp <UINT_MAX/ 4.Signed-off-by Dan Carpenter <[email protected]>---
v2: correct a typo in the commit messagedrivers/gpu/drm/drm_dumb_buffers.c +0
-0
drivers/gpu/drm/drm_dumb_buffers.c int drm_mode_create_dumb_ioctl(struct drm_device *dev, · line 69
Quoted 6 lines
drivers/gpu/drm/drm_dumb_buffers.c +2
-1@@ -65,7 +65,8 @@ 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 >UINT_MAX -8 ) +return -EINVAL ;
#define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d)
both args->bpp and cpp are u32, so should we use U32_MAX to be typesafe?
drivers/gpu/drm/drm_dumb_buffers.c int drm_mode_create_dumb_ioctl(struct drm_device *dev, · line 71
Quoted 2 lines
drivers/gpu/drm/drm_dumb_buffers.c +0
-0cpp =DIV_ROUND_UP (args->bpp,8 );if (!cpp || cpp >0xffffffffU / args->width)
And these constants should also be U32_MAX?
-Chris
Quoted 22 lines · Chris Wilson
Quoting Dan Carpenter (2018-05-09 09:12:54)
Quoted 21 lines
There is a comment here which says that
DIV_ROUND_UP()can overflow 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" isUINT_MAX/ 8 and because of how we pickedargs->widththat means cpp <UINT_MAX/ 4.Signed-off-by Dan Carpenter <[email protected]>---
v2: correct a typo in the commit messagedrivers/gpu/drm/drm_dumb_buffers.c +0
-0@@ -65,7 +65,8 @@ int drm_mode_create_dumb_ioctl(struct drm_device *dev,return -EINVAL ;
Quoted 9 lines
drivers/gpu/drm/drm_dumb_buffers.c int drm_mode_create_dumb_ioctl(struct drm_device *dev, · line 69
Quoted 4 lines
drivers/gpu/drm/drm_dumb_buffers.c +2
-1/* overflow checks for 32bit size calculations */ -/* NOTE: DIV_ROUND_UP() can overflow */ +if (args->bpp >UINT_MAX -8 ) +return -EINVAL ;#define
__KERNEL_DIV_ROUND_UP(n, d)(((n) + (d) - 1) / (d)both
args->bppand cpp are u32, so should we useU32_MAXto be typesafe?
They will always be equivalent so it's just a style issue... I don't have a strong preference so I can use U32_MAX if that's prefered.
Quoted 4 lines
drivers/gpu/drm/drm_dumb_buffers.c int drm_mode_create_dumb_ioctl(struct drm_device *dev, · line 71
Quoted 2 lines
drivers/gpu/drm/drm_dumb_buffers.c +0
-0cpp =DIV_ROUND_UP (args->bpp,8 );if (!cpp || cpp >0xffffffffU / args->width)And these constants should also be
U32_MAX?
Yeah. I considered changing those but one never knows how much style to clean up... I'll redo in the respin.
regards,
dan carpenter
PATCH v2 1/1 drm/dumb-buffers: Integer overflow in drm_mode_create_ioctl()
In reply to Chris WilsonThere 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.
---
v2: additional cleanups
drivers/gpu/drm/drm_dumb_buffers.c +4 -3
@@ -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 */
Quoting Dan Carpenter (2018-05-16 15:00:26)
Quoted 7 lines
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;
Quoted 8 lines
The integer overflow in
DIV_ROUND_UP()means "cpp" isUINT_MAX/ 8 and because of how we pickedargs->widththat 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 the0xffffffffUreferences toU32_MAX.Signed-off-by Dan Carpenter <[email protected]>
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.
-Chris
Quoted 15 lines · Chris Wilson
Quoting Dan Carpenter (2018-05-16 15:00:26)
Quoted 14 lines
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat 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 the0xffffffffUreferences toU32_MAX.
Quoted 7 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>I agree with Daniel that the !cpp check after
DIV_ROUND_UPwas sufficient to guard the current code, but switching to a more idiomatic style of overflow checking has its benefits too. Plus I like havingU32_MAXto 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)
Quoted 24 lines · Chris Wilson
Quoted 15 lines
Quoting Dan Carpenter (2018-05-16 15:00:26)
Quoted 14 lines
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat 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 the0xffffffffUreferences toU32_MAX.Quoted 7 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>I agree with Daniel that the !cpp check after
DIV_ROUND_UPwas sufficient to guard the current code, but switching to a more idiomatic style of overflow checking has its benefits too. Plus I like havingU32_MAXto compare the type ranges against.
Quoted 5 lines
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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 31 lines · Chris Wilson
Quoting Dan Carpenter (2018-05-16 15:52:57)
Quoted 24 lines · Chris Wilson
Quoted 15 lines
Quoting Dan Carpenter (2018-05-16 15:00:26)
Quoted 14 lines
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat 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 the0xffffffffUreferences toU32_MAX.Quoted 7 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>I agree with Daniel that the !cpp check after
DIV_ROUND_UPwas sufficient to guard the current code, but switching to a more idiomatic style of overflow checking has its benefits too. Plus I like havingU32_MAXto compare the type ranges against.Quoted 5 lines
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():
Quoted 4 lines
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)
Quoted 41 lines · Chris Wilson
Quoted 31 lines
Quoting Dan Carpenter (2018-05-16 15:52:57)
Quoted 24 lines · Chris Wilson
Quoted 15 lines
Quoting Dan Carpenter (2018-05-16 15:00:26)
Quoted 14 lines
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat 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 the0xffffffffUreferences toU32_MAX.Quoted 7 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>I agree with Daniel that the !cpp check after
DIV_ROUND_UPwas sufficient to guard the current code, but switching to a more idiomatic style of overflow checking has its benefits too. Plus I like havingU32_MAXto compare the type ranges against.Quoted 5 lines
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():Quoted 4 lines
But we are talking about
cpp = (U32_MAX + 8) / 8No, 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:
Quoted 5 lines
#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 usU32_MAX. And finally we divide by "d" to get "U32_MAX/ 8".
D'oh. Thanks for spelling it out.
-Chris
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
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
Quoted 10 lines · Dan Carpenter
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat means cpp <UINT_MAX/ 4.
Quoted 7 lines
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 the0xffffffffUreferences toU32_MAX.Signed-off-by Dan Carpenter <[email protected]>---
v2: additional cleanups
Thanks a lot for respinning, applied to drm-misc-fixes. -Daniel
Quoted 26 lines
drivers/gpu/drm/drm_dumb_buffers.c +4
-3@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Quoted 29 lines · Dan Carpenter
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" isUINT_MAX/ 8 and because of how we pickedargs->widththat means cpp <UINT_MAX/ 4.Signed-off-by Dan Carpenter <[email protected]>---
Btw,DIV_ROUND_UP()integer overflows have been a recurring source of bugs so I have an unreleased static checker warning specific for that. This line triggers three warnings for me on my unreleased code:
drivers/gpu/drm/drm_dumb_buffers.c:69drm_mode_create_dumb_ioctl()warn: negative user subtract: 0-u32max - 1drivers/gpu/drm/drm_dumb_buffers.c:69drm_mode_create_dumb_ioctl()warn: potential integer overflow from user '(args->bpp) + (8)'drivers/gpu/drm/drm_dumb_buffers.c:69drm_mode_create_dumb_ioctl()warn: potential integer overflow in 'DIV_ROUND_UP'It's a pretty common idiom in the kernel to overflow and then test for it later so I'm not able to release this code because of the number of false positives that this idiom causes...
drivers/gpu/drm/drm_dumb_buffers.c +0
-0
drivers/gpu/drm/drm_dumb_buffers.c int drm_mode_create_dumb_ioctl(struct drm_device *dev, · line 71
Quoted 8 lines
drivers/gpu/drm/drm_dumb_buffers.c +2
-1@@ -65,7 +65,8 @@ 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 >UINT_MAX -8 ) +return -EINVAL ; cpp =DIV_ROUND_UP (args->bpp,8 );if (!cpp || cpp >0xffffffffU / args->width)
The !cpp check is now redundant, this was our minimal overflow check. Note that we only really care for cpp != 0 and that the size calculation doesn't overflow. Userspace specifying a completely bogus bpp value is ok otherwise (reasonable values only go up to about 128). So I think there's no security issue here.
Anyway, can you pls respin with the !cpp check removed? See also
commit 6a77e80e55cacace60ff03aa717a6d364a401d2b (HEAD -> stuff)
Author: Daniel Vetter <[email protected]> Date: Mon Apr 30 17:04:10 2018 +0200
backlight: remove obsolete comment for ->state
for context.
Thanks, Daniel
Quoted 5 lines
drivers/gpu/drm/drm_dumb_buffers.c +0
-0return -EINVAL ;_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch