bug report: potential integer overflow in validate_exec_list()

bug report: potential integer overflow in validate_exec_list()

[Dan Carpenter]

Hello Chris,

Is there an integer overflow in validate_exec_list()?

drivers/gpu/drm/i915/i915_gem.c
  3633          size_t length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry);
  3634  
  3635          if (!access_ok(VERIFY_READ, ptr, length))
  3636                  return -EFAULT;
  3637  

My concern is that if relocation_count is larger than 0x8000000 the
multiplication can wrap.

This code was added in 2549d6c2 "drm/i915: Avoid vmallocing a buffer for
the relocations"

regards,
dan carpenter

Re: bug report: potential integer overflow in validate_exec_list()

[Chris Wilson]

On Sat, 20 Nov 2010 21:32:07 +0300, Dan Carpenter <error27@gmail.com> wrote:
> Hello Chris,
> 
> Is there an integer overflow in validate_exec_list()?
> 
> drivers/gpu/drm/i915/i915_gem.c
>   3633          size_t length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry);
>   3634  
>   3635          if (!access_ok(VERIFY_READ, ptr, length))
>   3636                  return -EFAULT;
>   3637  
> 
> My concern is that if relocation_count is larger than 0x8000000 the
> multiplication can wrap.

Yes, it could. Not through normal use since relocation count can not be
more than buffer length, hence realistically capped at around 4k entries.
However... 

Thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: bug report: potential integer overflow in validate_exec_list()

[Dan Carpenter]

On Sun, Nov 21, 2010 at 09:23:46AM +0000, Chris Wilson wrote:
> On Sat, 20 Nov 2010 21:32:07 +0300, Dan Carpenter <error27@gmail.com> wrote:
> > Hello Chris,
> > 
> > Is there an integer overflow in validate_exec_list()?
> > 
> > drivers/gpu/drm/i915/i915_gem.c
> >   3633          size_t length = exec[i].relocation_count * sizeof(struct drm_i915_gem_relocation_entry);
> >   3634  
> >   3635          if (!access_ok(VERIFY_READ, ptr, length))
> >   3636                  return -EFAULT;
> >   3637  
> > 
> > My concern is that if relocation_count is larger than 0x8000000 the
> > multiplication can wrap.
> 
> Yes, it could. Not through normal use since relocation count can not be
> more than buffer length, hence realistically capped at around 4k entries.
> However... 
> 

If the user deliberately made it wrap to get past the access_ok() check
then it would just return -ENOENT in i915_gem_execbuffer_relocate()
right?

It doesn't look like there are any security implications but I just
wanted to be sure.

regards,
dan carpenter

Re: bug report: potential integer overflow in validate_exec_list()

[Chris Wilson]

On Mon, 22 Nov 2010 12:56:42 +0300, Dan Carpenter <error27@gmail.com> wrote:
> On Sun, Nov 21, 2010 at 09:23:46AM +0000, Chris Wilson wrote:
> > Yes, it could. Not through normal use since relocation count can not be
> > more than buffer length, hence realistically capped at around 4k entries.
> > However... 
> > 
> 
> If the user deliberately made it wrap to get past the access_ok() check
> then it would just return -ENOENT in i915_gem_execbuffer_relocate()
> right?
> 
> It doesn't look like there are any security implications but I just
> wanted to be sure.

I think it did have a security implication, because it would only validate
the first x bytes of the user pointer but then continue to read/write
beyond. It would have to be a fairly crafty user! I've queued a fix in
the drm-intel-fixes branch:

commit d1d788302e8c76e5138dfa61f4a5eee4f72a748f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Sun Nov 21 09:23:48 2010 +0000

    drm/i915: Prevent integer overflow when validating the execbuffer
    
    Commit 2549d6c2 removed the vmalloc used for temporary storage of the
    relocation lists used during execbuffer. However, our use of vmalloc was
    being protected by an integer overflow check which we do want to
    preserve!
    
    Reported-by: Dan Carpenter <error27@gmail.com>
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Many thanks,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre