Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752128AbdI0Wcm (ORCPT ); Wed, 27 Sep 2017 18:32:42 -0400 Received: from mga03.intel.com ([134.134.136.65]:24439 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752116AbdI0Wcl (ORCPT ); Wed, 27 Sep 2017 18:32:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,446,1500966000"; d="scan'208";a="904543530" Message-ID: <1506551546.2532.36.camel@linux.intel.com> Subject: Re: [PATCH v8 12/28] 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, 27 Sep 2017 15:32:26 -0700 In-Reply-To: <20170927114713.wbee7ze2ud2ekvbw@pd.tnic> References: <20170819002809.111312-1-ricardo.neri-calderon@linux.intel.com> <20170819002809.111312-13-ricardo.neri-calderon@linux.intel.com> <20170926104353.vmpxybv3v5immc56@pd.tnic> <1506486104.8286.142.camel@linux.intel.com> <20170927114713.wbee7ze2ud2ekvbw@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 Content-Length: 6293 Lines: 208 On Wed, 2017-09-27 at 13:47 +0200, Borislav Petkov wrote: > On Tue, Sep 26, 2017 at 09:21:44PM -0700, Ricardo Neri wrote: > > > > This is true except when we don't have an insn at all (well, it may > > be > > non-NULL but it will only contain garbage). The case to which I am > > referring is when we begin decoding our instruction. The first step > > is > > to copy_from_user the instruction and populate insn. For this we > > must > > calculate the linear address from where we copy using CS and rIP. > Where do we do that? UMIP emulation does it when evaluating if emulation is needed after a #GP(0). It copy_from_user into insn the code at rIP that caused the exception [1]. > > > > > Furthermore, in this only case we don't need to look at insn at all > > as > > the only register involved is rIP no segment override prefixes are > > allowed. > In any case, as it is now it sounds convoluted: you may or may not > have an insn, and yet you call get_overridden_seg_reg() on it but you > don't really need segment overrides because you only need CS and rIP > initially. The idea is that get_overridden_seg_reg() would implement the logic you just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when segment override prefixes are not allowed (i.e., valid insn with operand rDI and string instruction; and rIP) or needed (i.e., long mode, except if there are override prefixes for FS or GS); or INAT_SEG_REG_[CSDEFG]S otherwise.  Then resolve_seg_register() resolves the default segment if needed as per the value returned by get_overridden_seg_reg(). Summarizing, a more accurate function name for the intended behavior is get_overridden_seg_reg_if_any_or_needed(). > Sounds to me like this initial parsing should be done separately from > this function... I decided to put all the handling of segment override prefixes in a single function. Perhaps it could be split into two functions as follows(diff on top of my original patches): * Rename get_overridden_seg_reg top get_overridden_seg_reg_idx * Remove from get_overridden_seg_reg_idx checks for rIP and rDI... * Checks for rIP and rDI are done in a new function * Now resolve_seg_reg calls the two functions above to determine if it needs to resolve the default segment register index. @@ -77,24 +77,12 @@ static bool is_string_insn(struct insn *insn)   * INAT_SEG_REG_DEFAULT is returned if no segment override prefixes were found   * and the default segment register shall be used. -EINVAL in case of error.   */ -static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs, -   int regoff) +static int get_overridden_seg_reg_idx(struct insn *insn, struct pt_regs *regs, +       int regoff)  {   int idx = INAT_SEG_REG_DEFAULT;   int sel_overrides = 0, i;   - /* -  * Segment override prefixes should not be used for (E)IP.  -  * Check this case first as we might not have (and not needed  -  * at all) a valid insn structure to evaluate segment override  -  * prefixes. -  */ - if (regoff == offsetof(struct pt_regs, ip)) { - if (user_64bit_mode(regs)) - return INAT_SEG_REG_IGNORE; - else - return INAT_SEG_REG_DEFAULT; - } -   if (!insn)   return -EINVAL;   @@ -145,18 +133,32 @@ static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs,   /*  * More than one segment override prefix leads to undefined   * behavior.  */   } else if (sel_overrides > 1) {   return -EINVAL; - /* -  * Segment override prefixes are always ignored for string  -  * instructions -  * that involve the use the (E)DI register. -  */ - } else if ((regoff == offsetof(struct pt_regs, di)) && -    is_string_insn(insn)) { - return INAT_SEG_REG_DEFAULT;   }     return idx;  }   +static int use_seg_reg_overrides(struct insn *insn, int regoff) +{ + /* +  * Segment override prefixes should not be used for rIP. Check  +  * this case first as we might not have (and not needed at all) +  * a valid insn structure to evaluate segment override  +  * prefixes. +  */ + if (regoff == offsetof(struct pt_regs, ip)) + return 0; + + /* Subsequent checks require a valid insn. */ + if (!insn) + return -EINVAL; + + if ((regoff == offsetof(struct pt_regs, di)) && +    is_string_insn(insn)) + return 0; + + return 1; +} +  /**   * resolve_seg_register() - obtain segment register   * @insn: Instruction structure with segment override prefixes @@ -179,22 +181,20 @@ static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs,   */  static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)  { - int idx; - - idx = get_overridden_seg_reg(insn, regs, regoff); + int use_pfx_overrides;   - if (idx < 0) - return idx; - - if (idx == INAT_SEG_REG_IGNORE) - return idx; + use_pfx_overrides = use_seg_reg_overrides(insn, regoff); + if (use_pfx_overrides < 0) + return -EINVAL;   - if (idx != INAT_SEG_REG_DEFAULT) - return idx; + if (use_pfx_overrides == 0) + goto resolve_default_idx;   - if (!insn) - return -EINVAL; + return get_overridden_seg_reg_idx(insn, regs, regoff);   +resolve_default_idx: + 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: @@ -209,6 +209,9 @@ static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)    *  + CS for (E)IP.    */   + if (!insn) + return -EINVAL; +   switch (regoff) {   case offsetof(struct pt_regs, ax):   case offsetof(struct pt_regs, cx): Does this make sense? > > > > > I only used "(E)" (i.e., not the "(R|)" part) as these utility > > functions will deal mostly with protected mode, unless FS or GS are > > used in long mode. > eIP or rIP is simply much easier to type and parse. Those brackets, > not > really. Agreed. Then I will use rIP. > > > > > I only check for a NULL insn when needed (i.e., the contents of the > > instruction could change the used segment register). > ... and those if (!insn) tests sprinkled around simply make the code > unreadable and if we can get rid of them, we should. Sure, you are correct this will make code more readable. Thanks and BR, Ricardo [1]. https://github.com/ricardon/tip/blob/rneri/umip_v9/arch/x86/kernel /umip.c#L276