From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752698AbdJLBNF (ORCPT ); Wed, 11 Oct 2017 21:13:05 -0400 Received: from mga06.intel.com ([134.134.136.31]:47465 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751712AbdJLBNE (ORCPT ); Wed, 11 Oct 2017 21:13:04 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,363,1503385200"; d="scan'208";a="322269428" Message-ID: <1507770750.17492.34.camel@linux.intel.com> Subject: Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get segment selector From: Ricardo Neri To: Borislav Petkov Cc: Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , Andy Lutomirski , Peter Zijlstra , Andrew Morton , Brian Gerst , Chris Metcalf , Dave Hansen , Paolo Bonzini , Masami Hiramatsu , Huang Rui , Jiri Slaby , Jonathan Corbet , "Michael S. Tsirkin" , Paul Gortmaker , Vlastimil Babka , Chen Yucong , "Ravi V. Shankar" , Shuah Khan , linux-kernel@vger.kernel.org, x86@kernel.org, Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov Date: Wed, 11 Oct 2017 18:12:30 -0700 In-Reply-To: <20171010224144.wedursa2s3zpip43@pd.tnic> References: <1507089272-32733-1-git-send-email-ricardo.neri-calderon@linux.intel.com> <1507089272-32733-14-git-send-email-ricardo.neri-calderon@linux.intel.com> <20171010224144.wedursa2s3zpip43@pd.tnic> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2017-10-11 at 00:41 +0200, Borislav Petkov wrote: > On Tue, Oct 03, 2017 at 08:54:16PM -0700, Ricardo Neri wrote: > > > > When computing a linear address and segmentation is used, we need to know > > the base address of the segment involved in the computation. In most of > > the cases, the segment base address will be zero as in USER_DS/USER32_DS. > ... > > > > > --- > >  arch/x86/include/asm/inat.h |  10 ++ > >  arch/x86/lib/insn-eval.c    | 321 > > ++++++++++++++++++++++++++++++++++++++++++++ > >  2 files changed, 331 insertions(+) > Ok, some more fixes ontop. I carved out the code under the > resolve_default_idx: label into a separate function. This made > resolve_seg_reg() pretty-much trivial to follow. Also renamed some > functions and variables to better denote what they do. Thanks! I will take your changes. > > Please add > > Improvements-by: Borislav Petkov > > to your commit message if you use this. Thanks. Will do. > > --- > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 77b48f99d73a..d02b94ace0f1 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -49,7 +49,7 @@ static bool is_string_insn(struct insn *insn) >  } >   >  /** > - * get_overridden_seg_reg_idx() - obtain segment register override index > + * get_seg_reg_override_idx() - obtain segment register override index >   * @insn: Instruction with segment override prefixes >   * >   * Inspect the instruction prefixes and find segment overrides, if any. > @@ -62,10 +62,10 @@ static bool is_string_insn(struct insn *insn) >   * >   * -EINVAL in case of error. >   */ > -static int get_overridden_seg_reg_idx(struct insn *insn) > +static int get_seg_reg_override_idx(struct insn *insn) >  { >   int idx = INAT_SEG_REG_DEFAULT; > - int sel_overrides = 0, i; > + int num_overrides = 0, i; >   >   if (!insn) >   return -EINVAL; > @@ -80,41 +80,41 @@ static int get_overridden_seg_reg_idx(struct insn *insn) >   switch (attr) { >   case INAT_MAKE_PREFIX(INAT_PFX_CS): >   idx = INAT_SEG_REG_CS; > - sel_overrides++; > + num_overrides++; >   break; >   case INAT_MAKE_PREFIX(INAT_PFX_SS): >   idx = INAT_SEG_REG_SS; > - sel_overrides++; > + num_overrides++; >   break; >   case INAT_MAKE_PREFIX(INAT_PFX_DS): >   idx = INAT_SEG_REG_DS; > - sel_overrides++; > + num_overrides++; >   break; >   case INAT_MAKE_PREFIX(INAT_PFX_ES): >   idx = INAT_SEG_REG_ES; > - sel_overrides++; > + num_overrides++; >   break; >   case INAT_MAKE_PREFIX(INAT_PFX_FS): >   idx = INAT_SEG_REG_FS; > - sel_overrides++; > + num_overrides++; >   break; >   case INAT_MAKE_PREFIX(INAT_PFX_GS): >   idx = INAT_SEG_REG_GS; > - sel_overrides++; > + num_overrides++; >   break; >   /* No default action needed. */ >   } >   } >   >   /* More than one segment override prefix leads to undefined behavior. > */ > - if (sel_overrides > 1) > + if (num_overrides > 1) >   return -EINVAL; >   >   return idx; >  } >   >  /** > - * allow_seg_reg_overrides() - check if segment override prefixes are allowed > + * check_seg_overrides() - check if segment override prefixes are allowed >   * @insn: Instruction with segment override prefixes >   * @regoff: Operand offset, in pt_regs, for which the check is > performed >   * > @@ -129,7 +129,7 @@ static int get_overridden_seg_reg_idx(struct insn *insn) >   * >   * -EINVAL in case of error. >   */ > -static int allow_seg_reg_overrides(struct insn *insn, int regoff) > +static int check_seg_overrides(struct insn *insn, int regoff) >  { >   /* >    * Segment override prefixes should not be used for rIP. It is not > @@ -148,6 +148,55 @@ static int allow_seg_reg_overrides(struct insn *insn, int > regoff) >   return 1; >  } >   > +static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int > off) > +{ Shouldn't this function check for a null insn since it is used here? > + if (user_64bit_mode(regs)) > + return INAT_SEG_REG_IGNORE; > + > + /* > +  * If we are here, we use the default segment register as described > +  * in the Intel documentation: > +  * > +  *  + DS for all references involving r[ABCD]X, and rSI. > +  *  + If used in a string instruction, ES for rDI. Otherwise, DS. > +  *  + AX, CX and DX are not valid register operands in 16-bit > addresses. > +  *    encodings but are valid for 32-bit and 64-bit encodings. > +  *  + -EDOM is reserved to identify for cases in which no register > +  *    is used (i.e., displacement-only addressing). Use DS. > +  *  + SS for (E)SP or (E)BP. > +  *  + CS for (E)IP. > +  */ > + switch (off) { > + case offsetof(struct pt_regs, ax): > + case offsetof(struct pt_regs, cx): > + case offsetof(struct pt_regs, dx): > + /* Need insn to verify address size. */ > + if (insn->addr_bytes == 2) > + return -EINVAL; > + > + case -EDOM: > + case offsetof(struct pt_regs, bx): > + case offsetof(struct pt_regs, si): > + return INAT_SEG_REG_DS; > + > + case offsetof(struct pt_regs, di): > + if (is_string_insn(insn)) > + return INAT_SEG_REG_ES; > + return INAT_SEG_REG_DS; > + > + case offsetof(struct pt_regs, bp): > + case offsetof(struct pt_regs, sp): > + return INAT_SEG_REG_SS; > + > + case offsetof(struct pt_regs, ip): > + return INAT_SEG_REG_CS; > + > + default: > + return -EINVAL; > + } > +} > + > + >  /** >   * resolve_seg_reg() - obtain segment register index >   * @insn: Instruction with operands > @@ -194,24 +243,24 @@ static int allow_seg_reg_overrides(struct insn *insn, > int regoff) >   */ >  static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int > regoff) >  { > - int use_pfx_overrides, idx; > + int ret, idx; >   > - use_pfx_overrides = allow_seg_reg_overrides(insn, regoff); > - if (use_pfx_overrides < 0) > - return use_pfx_overrides; > + ret = check_seg_overrides(insn, regoff); > + if (ret < 0) > + return ret; >   > - if (use_pfx_overrides == 0) > - goto resolve_default_idx; > + if (!ret) > + return resolve_default_seg(insn, regs, regoff); >   >   if (!insn) >   return -EINVAL; Could this check be removed? insn is not used for anything but passed to other functions that do perform this check. Thanks and BR, Ricardo