[4/4] drm/i915: fix relocation of secure buffers

Commit Message

Dave Gordon April 15, 2016, 11:32 a.m. UTC

There is a problem with the relocation of batches submitted with the
I915_EXEC_SECURE flag: although the batch itself will be mapped into the
GGTT, any relocations referring to it will use its address in the PPGTT,
which almost certainly won't be the same.

Hence a batch containing an MI_BATCH_BUFFER_START instruction that
references another part of the same batchbuffer will run correctly
in unprivileged mode, but will fail with a random jump when executed
in privileged mode.

This patch fixes the issue by changing eb_lookup_vmas() to take TWO
address space specifiers, one a new one for the batch itself and the
existing one used for all other buffer objects in the list.

This does not address the known limitation on batches *promoted* to
secure mode by the command parser, which are not allowed to contain
MI_BATCH_BUFFER_START or various other opcodes.

Discovered-by: Miguel Reche <[e-mail]>
Signed-off-by: Dave Gordon <[e-mail]>
Cc: Miguel Reche <[e-mail]>
---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Chris Wilson April 15, 2016, 11:43 a.m. UTC | #1

On Fri, Apr 15, 2016 at 12:32:57PM +0100, Dave Gordon wrote:
> There is a problem with the relocation of batches submitted with the
> I915_EXEC_SECURE flag: although the batch itself will be mapped into the
> GGTT, any relocations referring to it will use its address in the PPGTT,
> which almost certainly won't be the same.

This is incorrect. We can have and do use secure batches in the GGTT that
use ppGTT self relocations.
-Chris

Dave Gordon April 15, 2016, 12:24 p.m. UTC | #2

On 15/04/2016 12:43, Chris Wilson wrote:
> On Fri, Apr 15, 2016 at 12:32:57PM +0100, Dave Gordon wrote:
>> There is a problem with the relocation of batches submitted with the
>> I915_EXEC_SECURE flag: although the batch itself will be mapped into the
>> GGTT, any relocations referring to it will use its address in the PPGTT,
>> which almost certainly won't be the same.
> This is incorrect. We can have and do use secure batches in the GGTT that
> use ppGTT self relocations.
> -Chris

No, what I wrote is correct. A batch containing an MI_START_BATCH_BUFFER 
with a relocation description specifying the target as another part of 
the same batch will have the address of the batch in the PPGTT filled in 
there; Miguel has an example to demonstrate this. (And it's obvious from 
the code, relocation is completed before the GGTT mapping is created so 
can't put the GGTT address in the relocated entry).

Therefore, the secure batch cannot jump to another part of the buffer 
and remain in privileged mode.

OTOH it may ALSO be useful for the privileged batch to jump to 
UNPRIVILEGED subroutines, which would require the relocation to provide 
the PPGTT address (although I wouldn't expect that to be the default).

Maybe we need to extend the relocation interface so the batch can choose?

.Dave.

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3a60146..c0b4361 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -95,17 +95,19 @@  struct eb_vmas {
 	       struct drm_i915_gem_exec_object2 *exec,
 	       const struct drm_i915_gem_execbuffer2 *args,
 	       struct i915_address_space *vm,
+	       struct i915_address_space *vmb,
 	       struct drm_file *file)
 {
 	struct drm_i915_gem_object *obj;
 	struct list_head objects;
+	int n_obj = args->buffer_count;
 	int i, ret;
 
 	INIT_LIST_HEAD(&objects);
 	spin_lock(&file->table_lock);
 	/* Grab a reference to the object and release the lock so we can lookup
 	 * or create the VMA without using GFP_ATOMIC */
-	for (i = 0; i < args->buffer_count; i++) {
+	for (i = 0; i < n_obj; i++) {
 		obj = to_intel_bo(idr_find(&file->object_idr, exec[i].handle));
 		if (obj == NULL) {
 			spin_unlock(&file->table_lock);
@@ -128,14 +130,17 @@  struct eb_vmas {
 	}
 	spin_unlock(&file->table_lock);
 
-	i = 0;
-	while (!list_empty(&objects)) {
+	for (i = 0; !list_empty(&objects); --n_obj, ++i) {
 		struct i915_vma *vma;
 
 		obj = list_first_entry(&objects,
 				       struct drm_i915_gem_object,
 				       obj_exec_link);
 
+		/* Switch to vmb for the last item */
+		if (n_obj == 1)
+			vm = vmb;
+
 		/*
 		 * NOTE: We can leak any vmas created here when something fails
 		 * later on. But that's no issue since vma_unbind can deal with
@@ -164,7 +169,6 @@  struct eb_vmas {
 			hlist_add_head(&vma->exec_node,
 				       &eb->buckets[handle & eb->and]);
 		}
-		++i;
 	}
 
 	return 0;
@@ -861,7 +865,7 @@  static bool only_mappable_for_reloc(unsigned int flags)
 				  struct intel_context *ctx)
 {
 	struct drm_i915_gem_relocation_entry *reloc;
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *vmb;
 	struct i915_vma *vma;
 	bool need_relocs;
 	int *reloc_offset;
@@ -869,6 +873,7 @@  static bool only_mappable_for_reloc(unsigned int flags)
 	unsigned count = args->buffer_count;
 
 	vm = list_first_entry(&eb->vmas, struct i915_vma, exec_list)->vm;
+	vmb = eb_get_batch_vma(eb)->vm;
 
 	/* We may process another execbuffer during the unlock... */
 	while (!list_empty(&eb->vmas)) {
@@ -939,7 +944,7 @@  static bool only_mappable_for_reloc(unsigned int flags)
 
 	/* reacquire the objects */
 	eb_reset(eb);
-	ret = eb_lookup_vmas(eb, exec, args, vm, file);
+	ret = eb_lookup_vmas(eb, exec, args, vm, vmb, file);
 	if (ret)
 		goto err;
 
@@ -1452,7 +1457,7 @@  static bool only_mappable_for_reloc(unsigned int flags)
 	struct drm_i915_gem_exec_object2 shadow_exec_entry;
 	struct intel_engine_cs *engine;
 	struct intel_context *ctx;
-	struct i915_address_space *vm;
+	struct i915_address_space *vm, *vmb;
 	struct i915_execbuffer_params params_master; /* XXX: will be removed later */
 	struct i915_execbuffer_params *params = &params_master;
 	const u32 ctx_id = i915_execbuffer2_get_context_id(*args);
@@ -1520,6 +1525,12 @@  static bool only_mappable_for_reloc(unsigned int flags)
 	else
 		vm = &ggtt->base;
 
+	/* Secure batches must live in GGTT */
+	if (dispatch_flags & I915_DISPATCH_SECURE)
+		vmb = &dev_priv->ggtt.base;
+	else
+		vmb = vm;
+
 	memset(&params_master, 0x00, sizeof(params_master));
 
 	eb = eb_create(args);
@@ -1531,7 +1542,7 @@  static bool only_mappable_for_reloc(unsigned int flags)
 	}
 
 	/* Look up object handles */
-	ret = eb_lookup_vmas(eb, exec, args, vm, file);
+	ret = eb_lookup_vmas(eb, exec, args, vm, vmb, file);
 	if (ret)
 		goto err;