drm/i915: Fix command parser to validate multiple register access with the same command.

Until now the software command checker assumed that commands could
read or write at most a single register per packet.  This is not
necessarily the case, MI_LOAD_REGISTER_IMM expects a variable-length
list of offset/value pairs and writes them in sequence.  The previous
code would only check whether the first entry was valid, effectively
allowing userspace to write unrestricted registers of the MMIO space
by sending a multi-register write with a legal first register, with
potential security implications on Gen6 and 7 hardware.

Fix it by extending the drm_i915_cmd_descriptor table to represent
multi-register access and making validate_cmd() iterate for all
register offsets present in the command packet.

Signed-off-by: Francisco Jerez <currojerez@riseup.net>
Reviewed-by: Zhigang Gong <zhigang.gong@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
index 9605ff8f2fcdf4f2d3cefc806e96493311efc079..5fc49bbcdb9dee553f80c2ad6c0bb044b0ca8bef 100644 (file)
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
        CMD(  MI_SEMAPHORE_MBOX,                SMI,   !F,  0xFF,   R  ),
        CMD(  MI_STORE_DWORD_INDEX,             SMI,   !F,  0xFF,   R  ),
        CMD(  MI_LOAD_REGISTER_IMM(1),          SMI,   !F,  0xFF,   W,
-             .reg = { .offset = 1, .mask = 0x007FFFFC }               ),
+             .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 2 }    ),
        CMD(  MI_STORE_REGISTER_MEM(1),         SMI,   !F,  0xFF,   W | B,
              .reg = { .offset = 1, .mask = 0x007FFFFC },
              .bits = {{
@@ -934,7 +934,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs *ring)
 
 static bool check_cmd(const struct intel_engine_cs *ring,
                      const struct drm_i915_cmd_descriptor *desc,
-                     const u32 *cmd,
+                     const u32 *cmd, u32 length,
                      const bool is_master,
                      bool *oacontrol_set)
 {
@@ -950,38 +950,49 @@ static bool check_cmd(const struct intel_engine_cs *ring,
        }
 
        if (desc->flags & CMD_DESC_REGISTER) {
-               u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask;
-
                /*
-                * OACONTROL requires some special handling for writes. We
-                * want to make sure that any batch which enables OA also
-                * disables it before the end of the batch. The goal is to
-                * prevent one process from snooping on the perf data from
-                * another process. To do that, we need to check the value
-                * that will be written to the register. Hence, limit
-                * OACONTROL writes to only MI_LOAD_REGISTER_IMM commands.
+                * Get the distance between individual register offset
+                * fields if the command can perform more than one
+                * access at a time.
                 */
-               if (reg_addr == OACONTROL) {
-                       if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
-                               DRM_DEBUG_DRIVER("CMD: Rejected LRM to OACONTROL\n");
-                               return false;
+               const u32 step = desc->reg.step ? desc->reg.step : length;
+               u32 offset;
+
+               for (offset = desc->reg.offset; offset < length;
+                    offset += step) {
+                       const u32 reg_addr = cmd[offset] & desc->reg.mask;
+
+                       /*
+                        * OACONTROL requires some special handling for
+                        * writes. We want to make sure that any batch which
+                        * enables OA also disables it before the end of the
+                        * batch. The goal is to prevent one process from
+                        * snooping on the perf data from another process. To do
+                        * that, we need to check the value that will be written
+                        * to the register. Hence, limit OACONTROL writes to
+                        * only MI_LOAD_REGISTER_IMM commands.
+                        */
+                       if (reg_addr == OACONTROL) {
+                               if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
+                                       DRM_DEBUG_DRIVER("CMD: Rejected LRM to OACONTROL\n");
+                                       return false;
+                               }
+
+                               if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
+                                       *oacontrol_set = (cmd[offset + 1] != 0);
                        }
 
-                       if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
-                               *oacontrol_set = (cmd[2] != 0);
-               }
-
-               if (!valid_reg(ring->reg_table,
-                              ring->reg_count, reg_addr)) {
-                       if (!is_master ||
-                           !valid_reg(ring->master_reg_table,
-                                      ring->master_reg_count,
-                                      reg_addr)) {
-                               DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n",
-                                                reg_addr,
-                                                *cmd,
-                                                ring->id);
-                               return false;
+                       if (!valid_reg(ring->reg_table,
+                                      ring->reg_count, reg_addr)) {
+                               if (!is_master ||
+                                   !valid_reg(ring->master_reg_table,
+                                              ring->master_reg_count,
+                                              reg_addr)) {
+                                       DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n",
+                                                        reg_addr, *cmd,
+                                                        ring->id);
+                                       return false;
+                               }
                        }
                }
        }
@@ -1105,7 +1116,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
                        break;
                }
 
-               if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) {
+               if (!check_cmd(ring, desc, cmd, length, is_master,
+                              &oacontrol_set)) {
                        ret = -EINVAL;
                        break;
                }

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 46722f85ef761b0bf9dece7edf6192f8aca84acf..971fc330cc35a3dedf3eed3c4bbb8b1ec82971f1 100644 (file)
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2312,10 +2312,15 @@ struct drm_i915_cmd_descriptor {
         * Describes where to find a register address in the command to check
         * against the ring's register whitelist. Only valid if flags has the
         * CMD_DESC_REGISTER bit set.
+        *
+        * A non-zero step value implies that the command may access multiple
+        * registers in sequence (e.g. LRI), in that case step gives the
+        * distance in dwords between individual offset fields.
         */
        struct {
                u32 offset;
                u32 mask;
+               u32 step;
        } reg;
 
 #define MAX_CMD_DESC_BITMASKS 3