[Intel-gfx] [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command.
Daniel Vetter
daniel at ffwll.ch
Mon Jun 15 03:35:59 PDT 2015
On Tue, Jun 02, 2015 at 05:36:26PM +0800, Zhigang Gong wrote:
> The patchset LGTM and works well with beignet. The 80%+ performance regression issue in darktable also has been fixed
> after this patchset applied and enable the atomic in L3 at beignet side. So,
>
> Reviewed-by: Zhigang Gong <zhigang.gong at linux.intel.com>
All three merged. Aside: Dont we need an increment for the cmd parser
version for userspace to be able to detect this?
And please follow up with a link to the beignet patches used to validate
these kernel patches for references.
Thanks, Daniel
>
> Thanks,
> Zhigang Gong.
>
> > -----Original Message-----
> > From: Francisco Jerez [mailto:currojerez at riseup.net]
> > Sent: Friday, May 29, 2015 9:44 PM
> > To: intel-gfx at lists.freedesktop.org
> > Cc: Ville Syrjälä; Zhigang Gong; Brad Volkin
> > Subject: [PATCH 1/3] 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 at riseup.net>
> > ---
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 74
> > ++++++++++++++++++++--------------
> > drivers/gpu/drm/i915/i915_drv.h | 5 +++
> > 2 files changed, 48 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 61ae8ff..c4a5f73 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 = {{
> > @@ -939,7 +939,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)
> > {
> > @@ -955,38 +955,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;
> > + }
> > }
> > }
> > }
> > @@ -1110,7 +1121,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 8ae6f7f..3850288 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2228,10 +2228,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
> > --
> > 2.3.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list