From mboxrd@z Thu Jan 1 00:00:00 1970 From: Francisco Jerez Subject: Re: [PATCH 1/3] drm/i915: Fix command parser to validate multiple register access with the same command. Date: Tue, 02 Jun 2015 14:02:50 +0300 Message-ID: <87y4k29xut.fsf@riseup.net> References: <1432907055-8268-1-git-send-email-currojerez@riseup.net> <000901d09d17$99c5f380$cd51da80$@linux.intel.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0069999375==" Return-path: Received: from mx1.riseup.net (mx1.riseup.net [198.252.153.129]) by gabe.freedesktop.org (Postfix) with ESMTP id EB4076E3D7 for ; Tue, 2 Jun 2015 04:03:54 -0700 (PDT) In-Reply-To: <000901d09d17$99c5f380$cd51da80$@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Zhigang Gong , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============0069999375== Content-Type: multipart/signed; boundary="==-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --==-=-= Content-Type: multipart/mixed; boundary="=-=-=" --=-=-= Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Zhigang Gong writes: > The patchset LGTM and works well with beignet. The 80%+ performance regre= ssion 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 > > Thanks, > Zhigang Gong. > Thanks! >> -----Original Message----- >> From: Francisco Jerez [mailto:currojerez@riseup.net] >> Sent: Friday, May 29, 2015 9:44 PM >> To: intel-gfx@lists.freedesktop.org >> Cc: Ville Syrj=C3=A4l=C3=A4; Zhigang Gong; Brad Volkin >> Subject: [PATCH 1/3] drm/i915: Fix command parser to validate multiple >> register access with the same command. >>=20 >> 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 unres= tricted >> registers of the MMIO space by sending a multi-register write with a leg= al first >> register, with potential security implications on Gen6 and 7 hardware. >>=20 >> 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. >>=20 >> Signed-off-by: Francisco Jerez >> --- >> drivers/gpu/drm/i915/i915_cmd_parser.c | 74 >> ++++++++++++++++++++-------------- >> drivers/gpu/drm/i915/i915_drv.h | 5 +++ >> 2 files changed, 48 insertions(+), 31 deletions(-) >>=20 >> 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[] =3D { >> 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 =3D { .offset =3D 1, .mask =3D 0x007FFFFC } ), >> + .reg =3D { .offset =3D 1, .mask =3D 0x007FFFFC, .step =3D 2 } = ), >> CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, >> W | B, >> .reg =3D { .offset =3D 1, .mask =3D 0x007FFFFC }, >> .bits =3D {{ >> @@ -939,7 +939,7 @@ bool i915_needs_cmd_parser(struct intel_engine_cs >> *ring) >>=20 >> 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, >> } >>=20 >> if (desc->flags & CMD_DESC_REGISTER) { >> - u32 reg_addr =3D 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 =3D=3D OACONTROL) { >> - if (desc->cmd.value =3D=3D MI_LOAD_REGISTER_MEM) { >> - DRM_DEBUG_DRIVER("CMD: Rejected LRM to >> OACONTROL\n"); >> - return false; >> + const u32 step =3D desc->reg.step ? desc->reg.step : length; >> + u32 offset; >> + >> + for (offset =3D desc->reg.offset; offset < length; >> + offset +=3D step) { >> + const u32 reg_addr =3D 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 =3D=3D OACONTROL) { >> + if (desc->cmd.value =3D=3D MI_LOAD_REGISTER_MEM) { >> + DRM_DEBUG_DRIVER("CMD: Rejected LRM to >> OACONTROL\n"); >> + return false; >> + } >> + >> + if (desc->cmd.value =3D=3D MI_LOAD_REGISTER_IMM(1)) >> + *oacontrol_set =3D (cmd[offset + 1] !=3D 0); >> } >>=20 >> - if (desc->cmd.value =3D=3D MI_LOAD_REGISTER_IMM(1)) >> - *oacontrol_set =3D (cmd[2] !=3D 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=3D%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=3D%d)\n", >> + reg_addr, *cmd, >> + ring->id); >> + return false; >> + } >> } >> } >> } >> @@ -1110,7 +1121,8 @@ int i915_parse_cmds(struct intel_engine_cs *ring, >> break; >> } >>=20 >> - if (!check_cmd(ring, desc, cmd, is_master, &oacontrol_set)) { >> + if (!check_cmd(ring, desc, cmd, length, is_master, >> + &oacontrol_set)) { >> ret =3D -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; >>=20 >> #define MAX_CMD_DESC_BITMASKS 3 >> -- >> 2.3.5 --=-=-=-- --==-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iF4EAREIAAYFAlVtjVoACgkQg5k4nX1Sv1tlsgD+K0xGAvNRcJwt+c0H66jlJnIa UiapyrXIbXirr48xMywBAIQ23EwLr6ESxCwuAJwFXDeC5/hYdH86pVdVBfvw5Vff =dfhf -----END PGP SIGNATURE----- --==-=-=-- --===============0069999375== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0 cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK --===============0069999375==--