On Thu, Apr 30, 2015 at 12:32:15PM +0100, Rebecca N. Palmer wrote: > i915_parse_cmds returns -EACCES on chained batches, which "tells the > caller to abort and dispatch the workload as a non-secure batch", > but the mechanism implementing that was broken when > flags |= I915_DISPATCH_SECURE was moved from i915_gem_execbuffer_parse > to i915_gem_do_execbuffer (17cabf571e50677d980e9ab2a43c5f11213003ae): > i915_gem_execbuffer_parse returns the original batch_obj in this case, > and i915_gem_do_execbuffer doesn't check for that. > > Is this being made secure some other way (in which case the obsolete > comments should probably be removed), or is this a security hole? > > Warning: this is my first kernel patch, and has not been tested yet.
Looks really nice tbh and seems to fix a regression that the igt testsuite caught (gem_cmd_parse/chained-batches). Thanks a lot. Mika has some minor review comments, with those address I'll pull this in. Thanks, Daniel > Signed-off-by: Rebecca Palmer <[email protected]> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c > @@ -1398,7 +1398,7 @@ i915_gem_do_execbuffer(struct drm_device > { > struct drm_i915_private *dev_priv = dev->dev_private; > struct eb_vmas *eb; > - struct drm_i915_gem_object *batch_obj; > + struct drm_i915_gem_object *batch_obj, *orig_batch_obj; > struct drm_i915_gem_exec_object2 shadow_exec_entry; > struct intel_engine_cs *ring; > struct intel_context *ctx; > @@ -1511,7 +1511,7 @@ i915_gem_do_execbuffer(struct drm_device > goto err; > > /* take note of the batch buffer before we might reorder the lists */ > - batch_obj = eb_get_batch(eb); > + orig_batch_obj = eb_get_batch(eb); > > /* Move the objects en-masse into the GTT, evicting if necessary. */ > need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0; > @@ -1533,7 +1533,7 @@ i915_gem_do_execbuffer(struct drm_device > } > > /* Set the pending read domains for the batch buffer to COMMAND */ > - if (batch_obj->base.pending_write_domain) { > + if (orig_batch_obj->base.pending_write_domain) { > DRM_DEBUG("Attempting to use self-modifying batch buffer\n"); > ret = -EINVAL; > goto err; > @@ -1543,7 +1543,7 @@ i915_gem_do_execbuffer(struct drm_device > batch_obj = i915_gem_execbuffer_parse(ring, > &shadow_exec_entry, > eb, > - batch_obj, > + orig_batch_obj, > args->batch_start_offset, > args->batch_len, > file->is_master); > @@ -1559,7 +1559,7 @@ i915_gem_do_execbuffer(struct drm_device > * don't want that set when the command parser is > * enabled. > */ > - if (USES_PPGTT(dev)) > + if (USES_PPGTT(dev) && batch_obj!=orig_batch_obj) > dispatch_flags |= I915_DISPATCH_SECURE; > > exec_start = 0; > > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
