drm/i915/cmdparser: Add support for backward jumps
authorJon Bloomfield <jon.bloomfield@intel.com>
Thu, 20 Sep 2018 16:58:36 +0000 (09:58 -0700)
committerGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Tue, 12 Nov 2019 18:18:56 +0000 (19:18 +0100)
commit f8c08d8faee5567803c8c533865296ca30286bbf upstream.

To keep things manageable, the pre-gen9 cmdparser does not
attempt to track any form of nested BB_START's. This did not
prevent usermode from using nested starts, or even chained
batches because the cmdparser is not strictly enforced pre gen9.

Instead, the existence of a nested BB_START would cause the batch
to be emitted in insecure mode, and any privileged capabilities
would not be available.

For Gen9, the cmdparser becomes mandatory (for BCS at least), and
so not providing any form of nested BB_START support becomes
overly restrictive. Any such batch will simply not run.

We make heavy use of backward jumps in igt, and it is much easier
to add support for this restricted subset of nested jumps, than to
rewrite the whole of our test suite to avoid them.

Add the required logic to support limited backward jumps, to
instructions that have already been validated by the parser.

Note that it's not sufficient to simply approve any BB_START
that jumps backwards in the buffer because this would allow an
attacker to embed a rogue instruction sequence within the
operand words of a harmless instruction (say LRI) and jump to
that.

We introduce a bit array to track every instr offset successfully
validated, and test the target of BB_START against this. If the
target offset hits, it is re-written to the same offset in the
shadow buffer and the BB_START cmd is allowed.

Note: This patch deliberately ignores checkpatch issues in the
cmdtables, in order to match the style of the surrounding code.
We'll correct the entire file in one go in a later patch.

v2: set dispatch secure late (Mika)
v3: rebase (Mika)
v4: Clear whitelist on each parse
Minor review updates (Chris)
v5: Correct backward jump batching
v6: fix compilation error due to struct eb shuffle (Mika)

Signed-off-by: Jon Bloomfield <jon.bloomfield@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Takashi Iwai <tiwai@suse.de>
Cc: Tyler Hicks <tyhicks@canonical.com>
Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Chris Wilson <chris.p.wilson@intel.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
drivers/gpu/drm/i915/i915_cmd_parser.c
drivers/gpu/drm/i915/i915_drv.h
drivers/gpu/drm/i915/i915_gem_context.c
drivers/gpu/drm/i915/i915_gem_context.h
drivers/gpu/drm/i915/i915_gem_execbuffer.c

index 1ff3c8fd228fb191a33c9364ab88ac60a5dc020d..23d220fbca5fe48be1a5dec158a19b2a6e23e2eb 100644 (file)
@@ -481,6 +481,19 @@ static const struct drm_i915_cmd_descriptor gen9_blt_cmds[] = {
              .reg = { .offset = 1, .mask = 0x007FFFFC }               ),
        CMD(  MI_LOAD_REGISTER_REG,             SMI,    !F,  0xFF,  W,
              .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 }    ),
+
+       /*
+        * We allow BB_START but apply further checks. We just sanitize the
+        * basic fields here.
+        */
+#define MI_BB_START_OPERAND_MASK   GENMASK(SMI-1, 0)
+#define MI_BB_START_OPERAND_EXPECT (MI_BATCH_PPGTT_HSW | 1)
+       CMD(  MI_BATCH_BUFFER_START_GEN8,       SMI,    !F,  0xFF,  B,
+             .bits = {{
+                       .offset = 0,
+                       .mask = MI_BB_START_OPERAND_MASK,
+                       .expected = MI_BB_START_OPERAND_EXPECT,
+             }},                                                      ),
 };
 
 static const struct drm_i915_cmd_descriptor noop_desc =
@@ -1292,15 +1305,113 @@ static bool check_cmd(const struct intel_engine_cs *engine,
        return true;
 }
 
+static int check_bbstart(const struct i915_gem_context *ctx,
+                        u32 *cmd, u32 offset, u32 length,
+                        u32 batch_len,
+                        u64 batch_start,
+                        u64 shadow_batch_start)
+{
+       u64 jump_offset, jump_target;
+       u32 target_cmd_offset, target_cmd_index;
+
+       /* For igt compatibility on older platforms */
+       if (CMDPARSER_USES_GGTT(ctx->i915)) {
+               DRM_DEBUG("CMD: Rejecting BB_START for ggtt based submission\n");
+               return -EACCES;
+       }
+
+       if (length != 3) {
+               DRM_DEBUG("CMD: Recursive BB_START with bad length(%u)\n",
+                         length);
+               return -EINVAL;
+       }
+
+       jump_target = *(u64*)(cmd+1);
+       jump_offset = jump_target - batch_start;
+
+       /*
+        * Any underflow of jump_target is guaranteed to be outside the range
+        * of a u32, so >= test catches both too large and too small
+        */
+       if (jump_offset >= batch_len) {
+               DRM_DEBUG("CMD: BB_START to 0x%llx jumps out of BB\n",
+                         jump_target);
+               return -EINVAL;
+       }
+
+       /*
+        * This cannot overflow a u32 because we already checked jump_offset
+        * is within the BB, and the batch_len is a u32
+        */
+       target_cmd_offset = lower_32_bits(jump_offset);
+       target_cmd_index = target_cmd_offset / sizeof(u32);
+
+       *(u64*)(cmd + 1) = shadow_batch_start + target_cmd_offset;
+
+       if (target_cmd_index == offset)
+               return 0;
+
+       if (ctx->jump_whitelist_cmds <= target_cmd_index) {
+               DRM_DEBUG("CMD: Rejecting BB_START - truncated whitelist array\n");
+               return -EINVAL;
+       } else if (!test_bit(target_cmd_index, ctx->jump_whitelist)) {
+               DRM_DEBUG("CMD: BB_START to 0x%llx not a previously executed cmd\n",
+                         jump_target);
+               return -EINVAL;
+       }
+
+       return 0;
+}
+
+static void init_whitelist(struct i915_gem_context *ctx, u32 batch_len)
+{
+       const u32 batch_cmds = DIV_ROUND_UP(batch_len, sizeof(u32));
+       const u32 exact_size = BITS_TO_LONGS(batch_cmds);
+       u32 next_size = BITS_TO_LONGS(roundup_pow_of_two(batch_cmds));
+       unsigned long *next_whitelist;
+
+       if (CMDPARSER_USES_GGTT(ctx->i915))
+               return;
+
+       if (batch_cmds <= ctx->jump_whitelist_cmds) {
+               memset(ctx->jump_whitelist, 0, exact_size * sizeof(u32));
+               return;
+       }
+
+again:
+       next_whitelist = kcalloc(next_size, sizeof(long), GFP_KERNEL);
+       if (next_whitelist) {
+               kfree(ctx->jump_whitelist);
+               ctx->jump_whitelist = next_whitelist;
+               ctx->jump_whitelist_cmds =
+                       next_size * BITS_PER_BYTE * sizeof(long);
+               return;
+       }
+
+       if (next_size > exact_size) {
+               next_size = exact_size;
+               goto again;
+       }
+
+       DRM_DEBUG("CMD: Failed to extend whitelist. BB_START may be disallowed\n");
+       memset(ctx->jump_whitelist, 0,
+              BITS_TO_LONGS(ctx->jump_whitelist_cmds) * sizeof(u32));
+
+       return;
+}
+
 #define LENGTH_BIAS 2
 
 /**
  * i915_parse_cmds() - parse a submitted batch buffer for privilege violations
+ * @ctx: the context in which the batch is to execute
  * @engine: the engine on which the batch is to execute
  * @batch_obj: the batch buffer in question
- * @shadow_batch_obj: copy of the batch buffer in question
+ * @batch_start: Canonical base address of batch
  * @batch_start_offset: byte offset in the batch at which execution starts
  * @batch_len: length of the commands in batch_obj
+ * @shadow_batch_obj: copy of the batch buffer in question
+ * @shadow_batch_start: Canonical base address of shadow_batch_obj
  *
  * Parses the specified batch buffer looking for privilege violations as
  * described in the overview.
@@ -1308,13 +1419,17 @@ static bool check_cmd(const struct intel_engine_cs *engine,
  * Return: non-zero if the parser finds violations or otherwise fails; -EACCES
  * if the batch appears legal but should use hardware parsing
  */
-int intel_engine_cmd_parser(struct intel_engine_cs *engine,
+
+int intel_engine_cmd_parser(struct i915_gem_context *ctx,
+                           struct intel_engine_cs *engine,
                            struct drm_i915_gem_object *batch_obj,
-                           struct drm_i915_gem_object *shadow_batch_obj,
+                           u64 batch_start,
                            u32 batch_start_offset,
-                           u32 batch_len)
+                           u32 batch_len,
+                           struct drm_i915_gem_object *shadow_batch_obj,
+                           u64 shadow_batch_start)
 {
-       u32 *cmd, *batch_end;
+       u32 *cmd, *batch_end, offset = 0;
        struct drm_i915_cmd_descriptor default_desc = noop_desc;
        const struct drm_i915_cmd_descriptor *desc = &default_desc;
        bool needs_clflush_after = false;
@@ -1328,6 +1443,8 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
                return PTR_ERR(cmd);
        }
 
+       init_whitelist(ctx, batch_len);
+
        /*
         * We use the batch length as size because the shadow object is as
         * large or larger and copy_batch() will write MI_NOPs to the extra
@@ -1348,16 +1465,6 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
                        goto err;
                }
 
-               /*
-                * We don't try to handle BATCH_BUFFER_START because it adds
-                * non-trivial complexity. Instead we abort the scan and return
-                * and error to indicate that the batch is unsafe.
-                */
-               if (desc->cmd.value == MI_BATCH_BUFFER_START) {
-                       ret = -EACCES;
-                       goto err;
-               }
-
                if (desc->flags & CMD_DESC_FIXED)
                        length = desc->length.fixed;
                else
@@ -1377,7 +1484,21 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine,
                        goto err;
                }
 
+               if (desc->cmd.value == MI_BATCH_BUFFER_START) {
+                       ret = check_bbstart(ctx, cmd, offset, length,
+                                           batch_len, batch_start,
+                                           shadow_batch_start);
+
+                       if (ret)
+                               goto err;
+                       break;
+               }
+
+               if (ctx->jump_whitelist_cmds > offset)
+                       set_bit(offset, ctx->jump_whitelist);
+
                cmd += length;
+               offset += length;
                if  (cmd >= batch_end) {
                        DRM_DEBUG_DRIVER("CMD: Got to the end of the buffer w/o a BBE cmd!\n");
                        ret = -EINVAL;
index e50030c606656f0542ffd5bc557afe204823bfb3..e7e5f94e97f75bd07d8dfa37e614cf52b22a21fd 100644 (file)
@@ -3857,11 +3857,14 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type);
 int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv);
 void intel_engine_init_cmd_parser(struct intel_engine_cs *engine);
 void intel_engine_cleanup_cmd_parser(struct intel_engine_cs *engine);
-int intel_engine_cmd_parser(struct intel_engine_cs *engine,
+int intel_engine_cmd_parser(struct i915_gem_context *cxt,
+                           struct intel_engine_cs *engine,
                            struct drm_i915_gem_object *batch_obj,
-                           struct drm_i915_gem_object *shadow_batch_obj,
+                           u64 user_batch_start,
                            u32 batch_start_offset,
-                           u32 batch_len);
+                           u32 batch_len,
+                           struct drm_i915_gem_object *shadow_batch_obj,
+                           u64 shadow_batch_start);
 
 /* i915_perf.c */
 extern void i915_perf_init(struct drm_i915_private *dev_priv);
index 8afd2ce59b8d5050161bdd13611fcec4602a5321..3925a63c1661fcc250fd38871b7f88aec28aa2ff 100644 (file)
@@ -141,6 +141,8 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
                __i915_gem_object_release_unless_active(ce->state->obj);
        }
 
+       kfree(ctx->jump_whitelist);
+
        kfree(ctx->name);
        put_pid(ctx->pid);
 
@@ -321,6 +323,9 @@ __create_hw_context(struct drm_i915_private *dev_priv,
        else
                ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
 
+       ctx->jump_whitelist = NULL;
+       ctx->jump_whitelist_cmds = 0;
+
        return ctx;
 
 err_pid:
index 44688e22a5c22724906c39719b0ce11c76a1aaa6..b651c5f427b94f49bf5443856740c4490609bd61 100644 (file)
@@ -181,6 +181,12 @@ struct i915_gem_context {
        /** remap_slice: Bitmask of cache lines that need remapping */
        u8 remap_slice;
 
+       /** jump_whitelist: Bit array for tracking cmds during cmdparsing */
+       unsigned long *jump_whitelist;
+
+       /** jump_whitelist_cmds: No of cmd slots available */
+       u32 jump_whitelist_cmds;
+
        /** handles_vma: rbtree to look up our context specific obj/vma for
         * the user handle. (user handles are per fd, but the binding is
         * per vm, which may be one per context or shared with the global GTT)
index 12d44ea36d24a7dbd287151b6c3fe653f790aa73..d99d05a91032e6cf97462f0a6ced5493488a8307 100644 (file)
@@ -1927,7 +1927,6 @@ shadow_batch_pin(struct i915_execbuffer *eb, struct drm_i915_gem_object *obj)
        if (CMDPARSER_USES_GGTT(dev_priv)) {
                flags = PIN_GLOBAL;
                vm = &dev_priv->ggtt.base;
-               eb->batch_flags |= I915_DISPATCH_SECURE;
        } else if (eb->vm->has_read_only) {
                flags = PIN_USER;
                vm = eb->vm;
@@ -1944,6 +1943,8 @@ static struct i915_vma *eb_parse(struct i915_execbuffer *eb)
 {
        struct drm_i915_gem_object *shadow_batch_obj;
        struct i915_vma *vma;
+       u64 batch_start;
+       u64 shadow_batch_start;
        int err;
 
        shadow_batch_obj = i915_gem_batch_pool_get(&eb->engine->batch_pool,
@@ -1951,12 +1952,27 @@ static struct i915_vma *eb_parse(struct i915_execbuffer *eb)
        if (IS_ERR(shadow_batch_obj))
                return ERR_CAST(shadow_batch_obj);
 
-       err = intel_engine_cmd_parser(eb->engine,
+       vma = shadow_batch_pin(eb, shadow_batch_obj);
+       if (IS_ERR(vma))
+               goto out;
+
+       batch_start = gen8_canonical_addr(eb->batch->node.start) +
+                     eb->batch_start_offset;
+
+       shadow_batch_start = gen8_canonical_addr(vma->node.start);
+
+       err = intel_engine_cmd_parser(eb->ctx,
+                                     eb->engine,
                                      eb->batch->obj,
-                                     shadow_batch_obj,
+                                     batch_start,
                                      eb->batch_start_offset,
-                                     eb->batch_len);
+                                     eb->batch_len,
+                                     shadow_batch_obj,
+                                     shadow_batch_start);
+
        if (err) {
+               i915_vma_unpin(vma);
+
                /*
                 * Unsafe GGTT-backed buffers can still be submitted safely
                 * as non-secure.
@@ -1968,12 +1984,9 @@ static struct i915_vma *eb_parse(struct i915_execbuffer *eb)
                        vma = NULL;
                else
                        vma = ERR_PTR(err);
-               goto out;
-       }
 
-       vma = shadow_batch_pin(eb, shadow_batch_obj);
-       if (IS_ERR(vma))
                goto out;
+       }
 
        eb->vma[eb->buffer_count] = i915_vma_get(vma);
        eb->flags[eb->buffer_count] =
@@ -1984,6 +1997,9 @@ static struct i915_vma *eb_parse(struct i915_execbuffer *eb)
        eb->batch = vma;
        /* eb->batch_len unchanged */
 
+       if (CMDPARSER_USES_GGTT(eb->i915))
+               eb->batch_flags |= I915_DISPATCH_SECURE;
+
 out:
        i915_gem_object_unpin_pages(shadow_batch_obj);
        return vma;