Quoting Mika Kuoppala (2020-08-17 13:34:12)
Our variety of defined gpu commands have the actual command id field and possibly length and flags applied.
We did start to apply the mask during initialization of the cmd descriptors but forgot to also apply it on comparisons.
Fix comparisons in order to properly deny access with associated commands.
References: 926abff21a8f ("drm/i915/cmdparser: Ignore Length operands during command matching") Reported-by: Nicolai Stange nstange@suse.de Cc: stable@vger.kernel.org # v5.4+ Cc: Miroslav Benes mbenes@suse.cz Cc: Takashi Iwai tiwai@suse.de Cc: Tyler Hicks tyhicks@canonical.com Cc: Jon Bloomfield jon.bloomfield@intel.com Cc: Chris Wilson chris.p.wilson@intel.com Signed-off-by: Mika Kuoppala mika.kuoppala@linux.intel.com
drivers/gpu/drm/i915/i915_cmd_parser.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 372354d33f55..f2b0eb458d2d 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -1204,6 +1204,12 @@ static u32 *copy_batch(struct drm_i915_gem_object *dst_obj, return dst; } +static inline bool cmd_desc_is(const struct drm_i915_cmd_descriptor * const desc,
const u32 cmd)+{
return desc->cmd.value == (cmd & desc->cmd.mask);+}
static bool check_cmd(const struct intel_engine_cs *engine, const struct drm_i915_cmd_descriptor *desc, const u32 *cmd, u32 length) @@ -1242,24 +1248,24 @@ static bool check_cmd(const struct intel_engine_cs *engine, * allowed mask/value pair given in the whitelist entry. */ if (reg->mask) {
if (desc->cmd.value == MI_LOAD_REGISTER_MEM) {
if (cmd_desc_is(desc, MI_LOAD_REGISTER_MEM)) { DRM_DEBUG("CMD: Rejected LRM to masked register 0x%08X\n", reg_addr); return false;
}if (desc->cmd.value == MI_LOAD_REGISTER_REG) {
} else if (cmd_desc_is(desc, MI_LOAD_REGISTER_REG)) { DRM_DEBUG("CMD: Rejected LRR to masked register 0x%08X\n", reg_addr); return false;
}if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) &&
} else if (cmd_desc_is(desc, MI_LOAD_REGISTER_IMM(1)) && (offset + 2 > length || (cmd[offset + 1] & reg->mask) != reg->value)) { DRM_DEBUG("CMD: Rejected LRI to masked register 0x%08X\n", reg_addr); return false;
This needs to be split into a pass/fail.
The idea is that we want to allow an LRI into the register so long as it doesn't touch the masked bits. In this case we now fallthrough to the final rejection.
switch (desc->cmd.value) { case MI_INSTR(MI_LOAD_REGISTER_MEM, 0): /* I wish */ ... return false;
case MI_INSTR(MI_LOAD_REGISTER_REG, 0): ... return false;
case MI_INSTR(MI_LOAD_REGISTER_IMM, 0): if (offset + 2 > length || cmd[offset...] != allowed) { ... return false; } break; /* allow the LRI to update the other bits in reg */
default: .... return false; } --------------------------------------------------------------------- Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.