From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752715AbdFOSiA (ORCPT ); Thu, 15 Jun 2017 14:38:00 -0400 Received: from mga04.intel.com ([192.55.52.120]:62890 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751789AbdFOSh5 (ORCPT ); Thu, 15 Jun 2017 14:37:57 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,344,1493708400"; d="scan'208";a="1160981378" Message-ID: <1497551871.24288.169.camel@ranerica-desktop> Subject: Re: [PATCH v7 10/26] 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 , 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: Thu, 15 Jun 2017 11:37:51 -0700 In-Reply-To: <20170530103521.fb3wvp4crqapremh@pd.tnic> References: <20170505181724.55000-1-ricardo.neri-calderon@linux.intel.com> <20170505181724.55000-11-ricardo.neri-calderon@linux.intel.com> <20170530103521.fb3wvp4crqapremh@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 On Tue, 2017-05-30 at 12:35 +0200, Borislav Petkov wrote: > On Fri, May 05, 2017 at 11:17:08AM -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. > > However, it may be possible that a user space program defines its own > > segments via a local descriptor table. In such a case, the segment base > > address may not be zero .Thus, the segment base address is needed to > > calculate correctly the linear address. > > > > The segment selector to be used when computing a linear address is > > determined by either any of segment override prefixes in the > > instruction or inferred from the registers involved in the computation of > > the effective address; in that order. Also, there are cases when the > > overrides shall be ignored (code segments are always selected by the CS > > segment register; string instructions always use the ES segment register > > along with the EDI register). > > > > For clarity, this process can be split into two steps: resolving the > > relevant segment register to use and, once known, read its value to > > obtain the segment selector. > > > > The method to obtain the segment selector depends on several factors. In > > 32-bit builds, segment selectors are saved into the pt_regs structure > > when switching to kernel mode. The same is also true for virtual-8086 > > mode. In 64-bit builds, segmentation is mostly ignored, except when > > running a program in 32-bit legacy mode. In this case, CS and SS can be > > obtained from pt_regs. DS, ES, FS and GS can be read directly from > > the respective segment registers. > > > > Lastly, the only two segment registers that are not ignored in long mode > > are FS and GS. In these two cases, base addresses are obtained from the > > respective MSRs. > > > > 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/lib/insn-eval.c | 256 +++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 256 insertions(+) > > > > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > > index 1634762..0a496f4 100644 > > --- a/arch/x86/lib/insn-eval.c > > +++ b/arch/x86/lib/insn-eval.c > > @@ -9,6 +9,7 @@ > > #include > > #include > > #include > > +#include > > > > enum reg_type { > > REG_TYPE_RM = 0, > > @@ -33,6 +34,17 @@ enum string_instruction { > > SCASW_SCASD = 0xaf, > > }; > > > > +enum segment_register { > > + SEG_REG_INVAL = -1, > > + SEG_REG_IGNORE = 0, > > + SEG_REG_CS = 0x23, > > + SEG_REG_SS = 0x36, > > + SEG_REG_DS = 0x3e, > > + SEG_REG_ES = 0x26, > > + SEG_REG_FS = 0x64, > > + SEG_REG_GS = 0x65, > > +}; > > Yuck, didn't we talk about this already? I am sorry Borislav. I thought you agreed that I could use the values of the segment override prefixes to identify the segment registers [1]. > > Those are segment override prefixes so call them as such. > > #define SEG_OVR_PFX_CS 0x23 > #define SEG_OVR_PFX_SS 0x36 > ... > > and we already have those! > > arch/x86/include/asm/inat.h: > ... > #define INAT_PFX_CS 5 /* 0x2E */ > #define INAT_PFX_DS 6 /* 0x3E */ > #define INAT_PFX_ES 7 /* 0x26 */ > #define INAT_PFX_FS 8 /* 0x64 */ > #define INAT_PFX_GS 9 /* 0x65 */ > #define INAT_PFX_SS 10 /* 0x36 */ > > well, kinda, they're numbers there and not the actual prefix values. These numbers can 'translated' to the actual value of the prefixes via inat_get_opcode_attribute(). In my next version I am planning to use these function and reuse the aforementioned definitions. > > And then there's: > > arch/x86/kernel/uprobes.c::is_prefix_bad() which looks at some of those. > > Please add your defines to inat.h Will do. > and make that function is_prefix_bad() > use them instead of naked numbers. We need to pay attention to all those > different things needing to look at insn opcodes and not let them go > unwieldy by each defining and duplicating stuff. I have implemented this change and will be part of my next version. > > > /** > > * is_string_instruction - Determine if instruction is a string instruction > > * @insn: Instruction structure containing the opcode > > @@ -83,6 +95,250 @@ static bool is_string_instruction(struct insn *insn) > > } > > } > > > > +/** > > + * resolve_seg_register() - obtain segment register > > That function is still returning the segment override prefix and we use > *that* to determine the segment register. Once I add new definitions for the segment registers and reuse the existing definitions of the segment override prefixes this problem will be fixed. > > > + * @insn: Instruction structure with segment override prefixes > > + * @regs: Structure with register values as seen when entering kernel mode > > + * @regoff: Operand offset, in pt_regs, used to deterimine segment register > > + * > > + * The segment register to which an effective address refers depends on > > + * a) whether segment override prefixes must be ignored: always use CS when > > + * the register is (R|E)IP; always use ES when operand register is (E)DI with > > + * string instructions as defined in the Intel documentation. b) If segment > > + * overrides prefixes are used in the instruction instruction prefixes. C) Use > > + * the default segment register associated with the operand register. > > + * > > + * The operand register, regoff, is represented as the offset from the base of > > + * pt_regs. Also, regoff can be -EDOM for cases in which registers are not > > + * used as operands (e.g., displacement-only memory addressing). > > + * > > + * This function returns the segment register as value from an enumeration > > + * as per the conditions described above. Please note that this function > > + * does not return the value in the segment register (i.e., the segment > > + * selector). The segment selector needs to be obtained using > > + * get_segment_selector() and passing the segment register resolved by > > + * this function. > > + * > > + * Return: Enumerated segment register to use, among CS, SS, DS, ES, FS, GS, > > + * ignore (in 64-bit mode as applicable), or -EINVAL in case of error. > > + */ > > +static enum segment_register resolve_seg_register(struct insn *insn, > > + struct pt_regs *regs, > > + int regoff) > > +{ > > + int i; > > + int sel_overrides = 0; > > + int seg_register = SEG_REG_IGNORE; > > + > > + if (!insn) > > + return SEG_REG_INVAL; > > + > > + /* First handle cases when segment override prefixes must be ignored */ > > + if (regoff == offsetof(struct pt_regs, ip)) { > > + if (user_64bit_mode(regs)) > > + return SEG_REG_IGNORE; > > + else > > + return SEG_REG_CS; > > + return SEG_REG_CS; > > Simplify: > > if (user_64bit_mode(regs)) > return SEG_REG_IGNORE; > > return SEG_REG_CS; Will do. > > > + } > > + > > + /* > > + * If the (E)DI register is used with string instructions, the ES > > + * segment register is always used. > > + */ > > + if ((regoff == offsetof(struct pt_regs, di)) && > > + is_string_instruction(insn)) { > > + if (user_64bit_mode(regs)) > > + return SEG_REG_IGNORE; > > + else > > + return SEG_REG_ES; > > + return SEG_REG_CS; > > What is that second return actually supposed to do? This is not correct and I will remove it. Actually, will never run due to the if/else above it. Thanks for noticing it. > > > + } > > + > > + /* Then check if we have segment overrides prefixes*/ > > Missing space and fullstop: "... overrides prefixes. */" Will fix. > > > + for (i = 0; i < insn->prefixes.nbytes; i++) { > > + switch (insn->prefixes.bytes[i]) { > > + case SEG_REG_CS: > > + seg_register = SEG_REG_CS; > > + sel_overrides++; > > + break; > > + case SEG_REG_SS: > > + seg_register = SEG_REG_SS; > > + sel_overrides++; > > + break; > > + case SEG_REG_DS: > > + seg_register = SEG_REG_DS; > > + sel_overrides++; > > + break; > > + case SEG_REG_ES: > > + seg_register = SEG_REG_ES; > > + sel_overrides++; > > + break; > > + case SEG_REG_FS: > > + seg_register = SEG_REG_FS; > > + sel_overrides++; > > + break; > > + case SEG_REG_GS: > > + seg_register = SEG_REG_GS; > > + sel_overrides++; > > + break; > > + default: > > + return SEG_REG_INVAL; > > So SEG_REG_NONE or so? It is not invalid if it is not a segment override > prefix. Right, we can have more prefixes. We should need a default action as we are only looking for the segment override prefixes, as you mention. > > > + /* > > + * Having more than one segment override prefix leads to undefined > > + * behavior. If this is the case, return with error. > > + */ > > + if (sel_overrides > 1) > > + return SEG_REG_INVAL; > > Yuck, wrapping of -E value in a SEG_REG enum. Just return -EINVAL here > and make the function return an int, not that ugly enum. Will do. > > And the return convention should be straight-forward: default segment if > no prefix or ignored, -EINVAL if error and the actual override prefix if > present. Wouldn't this be ending up mixing the actual segment register and segment register overrides? I plan to have a function that parses the segment override prefixes and returns SEG_REG_CS/DS/ES/FS/GS or SEG_REG_IGNORE for long mode or SEG_REG_DEFAULT when the default segment register needs to be used. A separate function will determine what such default segment register is. Does this make sense? > > Also, that test should be *after* the user_64bit_mode() because in long > mode, segment overrides get ignored. IOW, those three if-tests around here > should be combined into a single one, i.e., something like this: > > if (64-bit) { > if (!FS || !GS) > ignore > else > return seg_override_pfx; <--- Yes, that variable should be called seg_override_pfx to denote what it is. Perhaps it can return what I have described above? > } else if (sel_overrides > 1) > -EINVAL > else if (sel_overrides) > return seg_override_pfx; > Will re-do these tests are you mention. > > + > > + if (sel_overrides == 1) { > > + /* > > + * If in long mode all segment registers but FS and GS are > > + * ignored. > > + */ > > + if (user_64bit_mode(regs) && !(seg_register == SEG_REG_FS || > > + seg_register == SEG_REG_GS)) > > + return SEG_REG_IGNORE; > > + > > + return seg_register; > > + } > > + > > + /* In long mode, all segment registers except FS and GS are ignored */ > > + if (user_64bit_mode(regs)) > > + return SEG_REG_IGNORE; > > + > > + /* > > + * Lastly, if no segment overrides were found, determine the default > > + * segment register as described in the Intel documentation: SS for > > + * (E)SP or (E)BP. DS for all data references, AX, CX and DX are not > > + * valid register operands in 16-bit address encodings. > > + * -EDOM is reserved to identify for cases in which no register is used > > + * the default segment register (displacement-only addressing). The > > + * default segment register used in these cases is DS. > > + */ > > + > > + switch (regoff) { > > + case offsetof(struct pt_regs, ax): > > + /* fall through */ > > + case offsetof(struct pt_regs, cx): > > + /* fall through */ > > + case offsetof(struct pt_regs, dx): > > + if (insn && insn->addr_bytes == 2) > > + return SEG_REG_INVAL; > > + case offsetof(struct pt_regs, di): > > + /* fall through */ > > + case -EDOM: > > + /* fall through */ > > + case offsetof(struct pt_regs, bx): > > + /* fall through */ > > + case offsetof(struct pt_regs, si): > > + return SEG_REG_DS; > > + case offsetof(struct pt_regs, bp): > > + /* fall through */ > > + case offsetof(struct pt_regs, sp): > > + return SEG_REG_SS; > > + case offsetof(struct pt_regs, ip): > > + return SEG_REG_CS; > > + default: > > + return SEG_REG_INVAL; > > + } > > So group all the fall through cases together so that you don't have this > dense block of code with "/* fall through */" on every other line. Will do. Thanks and BR, Ricardo