drm/i915: 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.

Is this being made secure some other way (in which case the obsolete comments should probably be removed), or is this a security hole?

Warning: this is my first kernel patch, and has not been tested yet.

Signed-off-by Rebecca Palmer <[email protected]>

drivers/gpu/drm/i915/i915_gem_execbuffer.c +4 -4

@@ -1398,7 +1398,7 @@ i915_gem_do_execbuffer(struct drm_device
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct eb_vmas *eb;
-	struct drm_i915_gem_object *batch_obj;
+	struct drm_i915_gem_object *batch_obj, *orig_batch_obj;
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *ring;
 	struct intel_context *ctx;

@@ -1511,7 +1511,7 @@ i915_gem_do_execbuffer(struct drm_device
 		goto err;
 
 	/* take note of the batch buffer before we might reorder the lists */
-	batch_obj = eb_get_batch(eb);
+	orig_batch_obj = eb_get_batch(eb);
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;

@@ -1533,7 +1533,7 @@ i915_gem_do_execbuffer(struct drm_device
 	}
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (batch_obj->base.pending_write_domain) {
+	if (orig_batch_obj->base.pending_write_domain) {
 		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
 		ret = -EINVAL;
 		goto err;

@@ -1543,7 +1543,7 @@ i915_gem_do_execbuffer(struct drm_device
 		batch_obj = i915_gem_execbuffer_parse(ring,
 						      &shadow_exec_entry,
 						      eb,
-						      batch_obj,
+						      orig_batch_obj,
 						      args->batch_start_offset,
 						      args->batch_len,
 						      file->is_master);

drivers/gpu/drm/i915/i915_gem_execbuffer.c +0 -0

 		exec_start = 0;
 

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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.

Is this being made secure some other way (in which case the obsolete comments should probably be removed), or is this a security hole?

Warning: this is my first kernel patch, and has not been tested yet.

Signed-off-by Rebecca Palmer <[email protected]>

drivers/gpu/drm/i915/i915_gem_execbuffer.c +5 -5

@@ -1398,7 +1398,7 @@ i915_gem_do_execbuffer(struct drm_device
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct eb_vmas *eb;
-	struct drm_i915_gem_object *batch_obj;
+	struct drm_i915_gem_object *batch_obj, *orig_batch_obj;
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *ring;
 	struct intel_context *ctx;

@@ -1511,7 +1511,7 @@ i915_gem_do_execbuffer(struct drm_device
 		goto err;
 
 	/* take note of the batch buffer before we might reorder the lists */
-	batch_obj = eb_get_batch(eb);
+	orig_batch_obj = eb_get_batch(eb);
 
 	/* Move the objects en-masse into the GTT, evicting if necessary. */
 	need_relocs = (args->flags & I915_EXEC_NO_RELOC) == 0;

@@ -1533,7 +1533,7 @@ i915_gem_do_execbuffer(struct drm_device
 	}
 
 	/* Set the pending read domains for the batch buffer to COMMAND */
-	if (batch_obj->base.pending_write_domain) {
+	if (orig_batch_obj->base.pending_write_domain) {
 		DRM_DEBUG("Attempting to use self-modifying batch buffer\n");
 		ret = -EINVAL;
 		goto err;

@@ -1543,7 +1543,7 @@ i915_gem_do_execbuffer(struct drm_device
 		batch_obj = i915_gem_execbuffer_parse(ring,
 						      &shadow_exec_entry,
 						      eb,
-						      batch_obj,
+						      orig_batch_obj,
 						      args->batch_start_offset,
 						      args->batch_len,
 						      file->is_master);

@@ -1559,7 +1559,7 @@ i915_gem_do_execbuffer(struct drm_device
 		 * don't want that set when the command parser is
 		 * enabled.
 		 */
-		if (USES_PPGTT(dev))
+		if (USES_PPGTT(dev) && batch_obj!=orig_batch_obj)
 			dispatch_flags |= I915_DISPATCH_SECURE;
 
 		exec_start = 0;

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

where cmdparser is disabled, batch_obj is
left dangling

Quoted 2 lines

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 <[email protected]>

drivers/gpu/drm/i915/i915_gem_execbuffer.c +5 -3

index 7ab63d9..2fb6dc1 100644
@@ -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 */

drivers/gpu/drm/i915/i915_gem_execbuffer.c +18 -10

+			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;

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

"Rebecca N. Palmer" <[email protected]> writes:

Hi,

Quoted 4 lines

Quoted 2 lines

where cmdparser is disabled, batch_obj is
left dangling

Sorry! Fixed now.

There is absolutely nothing to be sorry about.

Quoted 18 lines

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

---

Quoted 30 lines

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 <[email protected]>

drivers/gpu/drm/i915/i915_gem_execbuffer.c +3 -2

index 7ab63d9..2fb6dc1 100644
@@ -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)) {

Quoted 4 lines

drivers/gpu/drm/i915/i915_gem_execbuffer.c +2 -1

-			ret = PTR_ERR(batch_obj);
+		if (IS_ERR(parsed_batch_obj)) {
+			/* Batch rejected by parser, or an error

occurred */

This comment should be omitted as the rejection part is not valid in here and the error part is redudant. Daniel can squash it while applying I think.

For a first patch, stellar work!

Quoted 3 lines

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]

Repeating those tests[1] on linux-next 20150505 gave the same results, but having seen the intermittent "no window title bar" problem twice more, I now have to consider it a result of my patch[0], and hence withdraw it.

However, that doesn't mean there isn't a security problem here, only that we need a different way to fix it.

[0] http://lists.freedesktop.org/archives/intel-gfx/2015-April/065627.html
[1] http://lists.freedesktop.org/archives/intel-gfx/2015-May/065705.html