It is possible to wrap the counter used to allocate the buffer for
relocation copies. This could lead to heap writing overflows.
Signed-off-by: Kees Cook <[email protected]>
Reported-by: Pinkie Pie
Cc: [email protected]
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 752e399..62eaa99 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -585,7 +585,8 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
struct drm_i915_gem_object *obj;
bool need_relocs;
int *reloc_offset;
- int i, total, ret;
+ int ret;
+ unsigned int i, total;
int count = args->buffer_count;
/* We may process another execbuffer during the unlock... */
@@ -600,8 +601,13 @@ i915_gem_execbuffer_relocate_slow(struct drm_device *dev,
mutex_unlock(&dev->struct_mutex);
total = 0;
- for (i = 0; i < count; i++)
+ for (i = 0; i < count; i++) {
+ if (exec[i].relocation_count > UINT_MAX - total) {
+ mutex_lock(&dev->struct_mutex);
+ return -ENOMEM;
+ }
total += exec[i].relocation_count;
+ }
reloc_offset = drm_malloc_ab(count, sizeof(*reloc_offset));
reloc = drm_malloc_ab(total, sizeof(*reloc));
--
1.7.9.5
--
Kees Cook
Chrome OS Security
On Mon, Mar 11, 2013 at 12:27:16PM -0700, Kees Cook wrote:
> It is possible to wrap the counter used to allocate the buffer for
> relocation copies. This could lead to heap writing overflows.
Seems a sensible check, just in the wrong location. You need to do the
checking upfront in validate_exec_list() so that the error condition is
always hit and that the limits are applied consistently to all
execbuffers.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
On Mon, Mar 11, 2013 at 1:52 PM, Chris Wilson <[email protected]> wrote:
> On Mon, Mar 11, 2013 at 12:27:16PM -0700, Kees Cook wrote:
>> It is possible to wrap the counter used to allocate the buffer for
>> relocation copies. This could lead to heap writing overflows.
>
> Seems a sensible check, just in the wrong location. You need to do the
> checking upfront in validate_exec_list() so that the error condition is
> always hit and that the limits are applied consistently to all
> execbuffers.
I opted for it here because it kept it out of the fast path which
didn't need this check (it uses a list rather than an array). I will
move it to validate_exec_list().
Thanks!
-Kees
--
Kees Cook
Chrome OS Security