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

Rebecca N. Palmer
2015-05-08T16:52:56Z

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;