[Intel-gfx] [PATCH v2] drm/i915: Fix possible security hole in command parsing

[Daniel Vetter]

On Fri, May 08, 2015 at 05:04:48PM +0300, Mika Kuoppala wrote:
> "Rebecca N. Palmer" <rebecca_palmer at zoho.com> writes:
> 
> Hi,
> 
> >> where cmdparser is disabled, batch_obj is
> >> left dangling
> > Sorry!  Fixed now.
> >
> 
> There is absolutely nothing to be sorry about.
> 
> > This version also brings exec_start = 0 inside this check, as it
> > appears to be there because the copying (i915_cmd_parser.c:1054)
> > removes any offset the original might have had.
> >
> > When tested on next-20150508 (675b3fb), it passed my checks
> > (libva tests, vlc video, glxgears, beignet tests), and didn't
> > show the "missing window title bar" problem [0-1] in 3 attempts,
> > but given the intermittent nature of that I can't be sure.
> >
> > I still can't give useful i-g-t results, as it works on 3.16
> > but reports "GPU HANG" for most tests on 4.0 and (both patched and
> > unpatched) next (scripts/run-tests.sh at the recovery-mode
> > (single-user-mode) prompt, both i-g-t 1.10 and latest git).
> >
> > [0] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html
> > [1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065906.html
> >
> > ---

Just an side: git drops everything _below_ the --- line from the commit
message when applying the patch. Hence comments like the above should go
below that line, and the real commit message above. Also your commit
message for v2 doesn't mention how this bug was introduced, which is
crucial information. I've stitched something together.

> > i915_gem_execbuffer_parse returns the original batch_obj on batches
> > it can't check (currently, chained batches).  Don't set the secure
> > bit in this case.
> >
> > v2 (thanks to Mika Kuoppala):
> > Don't leave batch_obj unset when the parser is not run.
> > Only do exec_start = 0 on parsed batches.
> > Add comments.
> >
> > Signed-off-by: Rebecca Palmer <rebecca_palmer at zoho.com>
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 7ab63d9..2fb6dc1 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1540,28 +1540,38 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  	}
> >  
> >  	if (i915_needs_cmd_parser(ring) && args->batch_len) {
> > -		batch_obj = i915_gem_execbuffer_parse(ring,
> > +		struct drm_i915_gem_object *parsed_batch_obj;
> > +
> > +		parsed_batch_obj = i915_gem_execbuffer_parse(ring,
> >  						      &shadow_exec_entry,
> >  						      eb,
> >  						      batch_obj,
> >  						      args->batch_start_offset,
> >  						      args->batch_len,
> >  						      file->is_master);
> > -		if (IS_ERR(batch_obj)) {
> > -			ret = PTR_ERR(batch_obj);
> > +		if (IS_ERR(parsed_batch_obj)) {
> > +			/* Batch rejected by parser, or an error
> > occurred */
> 
> This comment should be omitted as the rejection part is not
> valid in here and the error part is redudant. Daniel can squash it while
> applying I think.

Done while applying.

> 
> For a first patch, stellar work!

Indeed. Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch