Commit 6a65c5b9 authored by Francisco Jerez's avatar Francisco Jerez Committed by Daniel Vetter

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: default avatarFrancisco Jerez <currojerez@riseup.net>
Reviewed-by: default avatarZhigang Gong <zhigang.gong@linux.intel.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
parent fcc0008f
...@@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = { ...@@ -123,7 +123,7 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, R ), CMD( MI_SEMAPHORE_MBOX, SMI, !F, 0xFF, R ),
CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ), CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W, 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, CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W | B,
.reg = { .offset = 1, .mask = 0x007FFFFC }, .reg = { .offset = 1, .mask = 0x007FFFFC },
.bits = {{ .bits = {{
...@@ -934,7 +934,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs *ring) ...@@ -934,7 +934,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs *ring)
static bool check_cmd(const struct intel_engine_cs *ring, static bool check_cmd(const struct intel_engine_cs *ring,
const struct drm_i915_cmd_descriptor *desc, const struct drm_i915_cmd_descriptor *desc,
const u32 *cmd, const u32 *cmd, u32 length,
const bool is_master, const bool is_master,
bool *oacontrol_set) bool *oacontrol_set)
{ {
...@@ -950,16 +950,27 @@ static bool check_cmd(const struct intel_engine_cs *ring, ...@@ -950,16 +950,27 @@ static bool check_cmd(const struct intel_engine_cs *ring,
} }
if (desc->flags & CMD_DESC_REGISTER) { if (desc->flags & CMD_DESC_REGISTER) {
u32 reg_addr = cmd[desc->reg.offset] & desc->reg.mask; /*
* Get the distance between individual register offset
* fields if the command can perform more than one
* access at a time.
*/
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 * OACONTROL requires some special handling for
* want to make sure that any batch which enables OA also * writes. We want to make sure that any batch which
* disables it before the end of the batch. The goal is to * enables OA also disables it before the end of the
* prevent one process from snooping on the perf data from * batch. The goal is to prevent one process from
* another process. To do that, we need to check the value * snooping on the perf data from another process. To do
* that will be written to the register. Hence, limit * that, we need to check the value that will be written
* OACONTROL writes to only MI_LOAD_REGISTER_IMM commands. * to the register. Hence, limit OACONTROL writes to
* only MI_LOAD_REGISTER_IMM commands.
*/ */
if (reg_addr == OACONTROL) { if (reg_addr == OACONTROL) {
if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
...@@ -968,7 +979,7 @@ static bool check_cmd(const struct intel_engine_cs *ring, ...@@ -968,7 +979,7 @@ static bool check_cmd(const struct intel_engine_cs *ring,
} }
if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1))
*oacontrol_set = (cmd[2] != 0); *oacontrol_set = (cmd[offset + 1] != 0);
} }
if (!valid_reg(ring->reg_table, if (!valid_reg(ring->reg_table,
...@@ -978,13 +989,13 @@ static bool check_cmd(const struct intel_engine_cs *ring, ...@@ -978,13 +989,13 @@ static bool check_cmd(const struct intel_engine_cs *ring,
ring->master_reg_count, ring->master_reg_count,
reg_addr)) { reg_addr)) {
DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n", DRM_DEBUG_DRIVER("CMD: Rejected register 0x%08X in command: 0x%08X (ring=%d)\n",
reg_addr, reg_addr, *cmd,
*cmd,
ring->id); ring->id);
return false; return false;
} }
} }
} }
}
if (desc->flags & CMD_DESC_BITMASK) { if (desc->flags & CMD_DESC_BITMASK) {
int i; int i;
...@@ -1105,7 +1116,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring, ...@@ -1105,7 +1116,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring,
break; break;
} }
if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) { if (!check_cmd(ring, desc, cmd, length, is_master,
&oacontrol_set)) {
ret = -EINVAL; ret = -EINVAL;
break; break;
} }
......
...@@ -2312,10 +2312,15 @@ struct drm_i915_cmd_descriptor { ...@@ -2312,10 +2312,15 @@ struct drm_i915_cmd_descriptor {
* Describes where to find a register address in the command to check * Describes where to find a register address in the command to check
* against the ring's register whitelist. Only valid if flags has the * against the ring's register whitelist. Only valid if flags has the
* CMD_DESC_REGISTER bit set. * 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 { struct {
u32 offset; u32 offset;
u32 mask; u32 mask;
u32 step;
} reg; } reg;
#define MAX_CMD_DESC_BITMASKS 3 #define MAX_CMD_DESC_BITMASKS 3
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment