[for,4.1] drm/i915: Don't clear exec_start if batch was not copied
diff mbox

Message ID 554CE99A.3030603@zoho.com
State New
Headers show

Commit Message

Rebecca N. Palmer May 8, 2015, 4:51 p.m. UTC
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).

Patch
diff mbox

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;