Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031836AbdDZWiJ (ORCPT ); Wed, 26 Apr 2017 18:38:09 -0400 Received: from mga04.intel.com ([192.55.52.120]:7025 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031805AbdDZWhr (ORCPT ); Wed, 26 Apr 2017 18:37:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.37,256,1488873600"; d="scan'208";a="94255950" Message-ID: <1493246264.36058.72.camel@ranerica-desktop> Subject: Re: [v6 PATCH 08/21] x86/insn-eval: Add utility function to get segment descriptor base address 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 , Alexandre Julliard , Stas Sergeev , Fenghua Yu , "Ravi V. Shankar" , Shuah Khan , linux-kernel@vger.kernel.org, x86@kernel.org, linux-msdos@vger.kernel.org, wine-devel@winehq.org, Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov Date: Wed, 26 Apr 2017 15:37:44 -0700 In-Reply-To: <20170420082533.hwc6eon3aom43qzo@pd.tnic> References: <20170308003254.27833-1-ricardo.neri-calderon@linux.intel.com> <20170308003254.27833-9-ricardo.neri-calderon@linux.intel.com> <20170420082533.hwc6eon3aom43qzo@pd.tnic> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5903 Lines: 160 On Thu, 2017-04-20 at 10:25 +0200, Borislav Petkov wrote: > On Tue, Mar 07, 2017 at 04:32:41PM -0800, Ricardo Neri wrote: > > With segmentation, the base address of the segment descriptor is needed > > to compute a linear address. The segment descriptor used in the address > > computation depends on either any segment override prefixes in the in the > > s/in the // I will fix this typo. > > > instruction or the default segment determined by the registers involved > > in the address computation. Thus, both the instruction as well as the > > register (specified as the offset from the base of pt_regs) are given as > > inputs, along with a boolean variable to select between override and > > default. > > > > The segment selector is determined by get_seg_selector with the inputs > > Please end function names with parentheses: get_seg_selector(). I will use parentheses. > > > described above. Once the selector is known the base address is > > known, ... Will fix. > > > determined. In protected mode, the selector is used to obtain the segment > > descriptor and then its base address. If in 64-bit user mode, the segment = > > base address is zero except when FS or GS are used. In virtual-8086 mode, > > the base address is computed as the value of the segment selector shifted 4 > > positions to the left. > > Good. > > > Cc: Dave Hansen > > Cc: Adam Buchbinder > > Cc: Colin Ian King > > Cc: Lorenzo Stoakes > > Cc: Qiaowei Ren > > Cc: Arnaldo Carvalho de Melo > > Cc: Masami Hiramatsu > > Cc: Adrian Hunter > > Cc: Kees Cook > > Cc: Thomas Garnier > > Cc: Peter Zijlstra > > Cc: Borislav Petkov > > Cc: Dmitry Vyukov > > Cc: Ravi V. Shankar > > Cc: x86@kernel.org > > Signed-off-by: Ricardo Neri > > --- > > arch/x86/include/asm/insn-eval.h | 2 ++ > > arch/x86/lib/insn-eval.c | 66 ++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 68 insertions(+) > > > > diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h > > index 754211b..b201742 100644 > > --- a/arch/x86/include/asm/insn-eval.h > > +++ b/arch/x86/include/asm/insn-eval.h > > @@ -15,5 +15,7 @@ void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_modrm_rm(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > int insn_get_reg_offset_sib_base(struct insn *insn, struct pt_regs *regs); > > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > + int regoff, bool use_default_seg); > > > > #endif /* _ASM_X86_INSN_EVAL_H */ > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 8608adf..383ca83 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -355,6 +355,72 @@ static int get_desc(unsigned short seg, struct desc_struct **desc) > > } > > > > /** > > + * insn_get_seg_base() - Obtain base address contained in descriptor > > + * @regs: Set of registers containing the segment selector > > + * @insn: Instruction structure with selector override prefixes > > + * @regoff: Operand offset, in pt_regs, of which the selector is needed > > + * @use_default_seg: Use the default segment instead of prefix overrides > > I'm wondering whether you really need that bool or you can deduce this > from pt_regs... I guess I'll see... > > > + * > > + * Obtain the base address of the segment descriptor as indicated by either > > + * any segment override prefixes contained in insn or the default segment > > + * applicable to the register indicated by regoff. regoff is specified as the > > + * offset in bytes from the base of pt_regs. > > + * > > + * Return: In protected mode, base address of the segment. It may be zero in > > + * certain cases for 64-bit builds and/or 64-bit applications. In virtual-8086 > > + * mode, the segment selector shifed 4 positions to the right. -1L in case of > > s/shifed/shifted/ I will correct the typo. > > > + * error. > > + */ > > +unsigned long insn_get_seg_base(struct pt_regs *regs, struct insn *insn, > > + int regoff, bool use_default_seg) > > +{ > > + struct desc_struct *desc; > > + unsigned short seg; > > + enum segment seg_type; > > + int ret; > > + > > + seg_type = resolve_seg_selector(insn, regoff, use_default_seg); > > <--- error handling. I will add it. > > And that's not really a "seg_type" but simply the "sel"-ector. I will update the variable names to reflect the fact that they are segment selectors. > And that > "enum segment" is not really a segment but an segment override prefixes > enum. Can we please get the nomenclature right first? I need a human-readable way of identifying what segment selector (in pt_regs, vm86regs or directly reading the segment registers) to use. Since there is a segment override prefix for all of them, I thought I could use them. Perhaps I can rename enum segment to enum segment_selector and comment that the values in the enum are those of the override prefixes. Would that be reasonable? > > > + > > + seg = get_segment_selector(regs, seg_type); > > s/seg/sel/ Will change. > > > + if (seg < 0) > > + return -1L; > > + > > + if (v8086_mode(regs)) > > + /* > > + * Base is simply the segment selector shifted 4 > > + * positions to the right. > > + */ > > + return (unsigned long)(seg << 4); > > + > > +#ifdef CONFIG_X86_64 > > + if (user_64bit_mode(regs)) { > > if (IS_ENABLED(CONFIG_X86_64) && user_64bit_mode(regs)) { I will change it. Thanks and BR, Ricardo