Re: [PATCH v2] drm/i915: Fix possible security hole in command parsing

Rebecca N. Palmer
2015-05-08T13:28:09Z

> where cmdparser is disabled, batch_obj is
> left dangling
Sorry! Fixed now.

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

---

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@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 */
+ 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.
- */
- dispatch_flags |= I915_DISPATCH_SECURE;
-
- exec_start = 0;
+ /* parsed_batch_obj == batch_obj means batch not fully parsed:
+ * accept, but don't promote to secure */
+
+ 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.
+ */
+ dispatch_flags |= I915_DISPATCH_SECURE;
+ exec_start = 0;
+ batch_obj = parsed_batch_obj;
+ }
}

batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;