Re: [PATCH for 4.1] drm/i915: Don't clear exec_start if batch was not copied

["Rebecca N. Palmer" <rebecca_palmer@zoho.com>]

i915_gem_execbuffer_parse returns the original batch_obj on batches
it can't check (currently, chained batches).  Don't clear offset
or set I915_DISPATCH_SECURE in this case.

Fixes 17cabf571e50677d980e9ab2a43c5f11213003ae.

Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com>
---
> > 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.
> [pushed without comment on this]

That makes this a bug in mainline as well, though I don't know of
any actual problems it causes.

(The security hole exists there too, but only with the declared-unsafe
parameter i915.enable_ppgtt=2, hence the change of title)

This fix was tested on 3e0283a (tip of Linus' tree), in the same way
as before (libva tests, vlc video, glxgears, beignet tests).

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index a3190e79..5ff8a64 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1548,33 +1548,39 @@ 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)) {
+			ret = PTR_ERR(parsed_batch_obj);
 			goto err;
 		}
 
-		/*
-		 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
-		 * bit from MI_BATCH_BUFFER_START commands issued in the
-		 * dispatch_execbuffer implementations. We specifically
-		 * don't want that set when the command parser is
-		 * enabled.
-		 *
-		 * FIXME: with aliasing ppgtt, buffers that should only
-		 * be in ggtt still end up in the aliasing ppgtt. remove
-		 * this check when that is fixed.
-		 */
-		if (USES_FULL_PPGTT(dev))
-			dispatch_flags |= I915_DISPATCH_SECURE;
-
-		exec_start = 0;
+		if (parsed_batch_obj != batch_obj) {
+			/*
+			 * Batch parsed and accepted:
+			 *
+			 * Set the DISPATCH_SECURE bit to remove the NON_SECURE
+			 * bit from MI_BATCH_BUFFER_START commands issued in
+			 * the dispatch_execbuffer implementations. We
+			 * specifically don't want that set on batches the
+			 * command parser has accepted.
+			 *
+			 * FIXME: with aliasing ppgtt, buffers that should only
+			 * be in ggtt still end up in the aliasing ppgtt.
+			 * remove USES_FULL_PPGTT check when that is fixed.
+			 */
+			if (USES_FULL_PPGTT(dev))
+				dispatch_flags |= I915_DISPATCH_SECURE;
+			exec_start = 0;
+			batch_obj = parsed_batch_obj;
+		}
 	}
 
 	batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx