From: Francisco Jerez Date: Fri, 29 May 2015 13:44:13 +0000 (+0300) Subject: drm/i915: Fix command parser to validate multiple register access with the same command. X-Git-Url: https://git.karo-electronics.de/?a=commitdiff_plain;h=8a389cac1fd7dd6411c7e586ab098938a75870e0;p=linux-beck.git 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 Reviewed-by: Zhigang Gong Signed-off-by: Daniel Vetter Signed-off-by: Jani Nikula --- diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 9605ff8f2fcd..5fc49bbcdb9d 100644 --- 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 72f5a3f9dbf2..542fac628b28 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2300,10 +2300,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