drm/i915: Fix possible security hole in command parsing

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.

Don't set the secure bit in this case to make sure such batches don't
run with elevated priviledges.

Signed-off-by: Rebecca Palmer <rebecca_palmer@zoho.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@intel.com>
[danvet: Stitch together commit message. Also remove a comment as
suggested by Mika. And style-align the comment while at it.]
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 7ab63d9d7dc56bb98d7118c3b1f2d14207662f22..560c79a8a43d5cac451ce2001ffac0578d89259c 100644 (file)
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1540,28 +1540,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.
+                * parsed_batch_obj == batch_obj means batch not fully parsed:
+                * Accept, but don't promote to secure.
                 */
-               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.
+                        */
+                       dispatch_flags |= I915_DISPATCH_SECURE;
+                       exec_start = 0;
+                       batch_obj = parsed_batch_obj;
+               }
        }
 
        batch_obj->base.pending_read_domains |= I915_GEM_DOMAIN_COMMAND;