Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752058AbdI2GG7 (ORCPT ); Fri, 29 Sep 2017 02:06:59 -0400 Received: from mga06.intel.com ([134.134.136.31]:14295 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751070AbdI2GG5 (ORCPT ); Fri, 29 Sep 2017 02:06:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.42,451,1500966000"; d="scan'208";a="1225084556" Message-ID: <1506665202.2532.118.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: Thu, 28 Sep 2017 23:06:42 -0700 In-Reply-To: <20170928093639.nwvohdd7h7i4htft@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> <1506551546.2532.36.camel@linux.intel.com> <20170928093639.nwvohdd7h7i4htft@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: 3413 Lines: 82 On Thu, 2017-09-28 at 11:36 +0200, Borislav Petkov wrote: > On Wed, Sep 27, 2017 at 03:32:26PM -0700, Ricardo Neri wrote: > > > > 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. > Ok, lemme see if we're talking the same thing. Your diff is linewrapped > so parsing that is hard. > > Do this > >         if (regoff == offsetof(struct pt_regs, ip)) { >                 if (user_64bit_mode(regs)) >                         return INAT_SEG_REG_IGNORE; >                 else >                         return INAT_SEG_REG_DEFAULT; >         } > > and all the other checking *before* you do insn_init(). Because you have > crazy stuff like: > >         if (seg_reg == INAT_SEG_REG_IGNORE) >                 return seg_reg; > > which shortcuts those functions and is simply clumsy and complicates > following the code. The mere fact that you have to call the function > "get_overridden_seg_reg_if_any_or_needed()" already tells you that that > function is doing too many things at once. > > When the function is called get_segment_register() then it should do > only that. And all the checking is done before or in wrappers. Yes, I realized this while I was typing. > > IOW, all the rIP checking and early return down the > insn_get_seg_base() -> resolve_seg_register() -> .. should be done > separately. Agreed now. > > *Then* you do insn_init() and hand it down to insn_get_seg_base() and > from now on you have a proper insn pointer which you hand around and > check for NULL only once, on function entry. I agree. In fact, insn_get_seg_base() does not need insn at all. All it needs is a INAT_SEG_REG_* index. This would make things clear. UMIP (and callers that need to copy_from_user code can do insn_get_seg_base(regs, INAT_SEG_REG_CS). No insn needed. In fact, it is only the insn_get_addr_ref_xx() family of functions that does need to inspect insn (which will be populated and valided) to determine the what registers are used as operands... and determine the applicable segment register. However, insn_get_addr_ref_xx() functions call insn_get_seg_base() several times each. Each time they would need to do: if (can_use_seg_override_prefixes(insn, regoff))     idx = get_overriden_seg_reg(insn, regs) else     idx = get_default_seg_reg() The pseudocode above looks like a resolve_reg_idx() to me. Then insn_get_addr_ref_xx() can call insn_get_seg_base(idx). > > Then your code flow is much simpler: first you take care of the case > where rIP doesn't do segment overrides and all the other cases are > handled by the normal path, with a proper struct insn. Do you think the pseudocode above addresses your concerns? *insn_get_seg_base() will take a INAT_SEG_REG_* index *insn_get_ref_xx() receives an initialized insn that can check for NULL value. *a reworked resolve_seg_reg_idx will clearly check if it can use segment override prefixes and obtain them. If not, it will use default values. Thanks and BR, Ricardo