Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S968046AbdIZKoE (ORCPT ); Tue, 26 Sep 2017 06:44:04 -0400 Received: from mx2.suse.de ([195.135.220.15]:53383 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S965144AbdIZKoC (ORCPT ); Tue, 26 Sep 2017 06:44:02 -0400 Date: Tue, 26 Sep 2017 12:43:53 +0200 From: Borislav Petkov To: Ricardo Neri 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, ricardo.neri@intel.com, Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov Subject: Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Message-ID: <20170926104353.vmpxybv3v5immc56@pd.tnic> References: <20170819002809.111312-1-ricardo.neri-calderon@linux.intel.com> <20170819002809.111312-13-ricardo.neri-calderon@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170819002809.111312-13-ricardo.neri-calderon@linux.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11136 Lines: 303 Hi, On Fri, Aug 18, 2017 at 05:27:53PM -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 | 278 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 288 insertions(+) so I did a bunch of simplifications on top, see if you agree: * we should always test for if (!insn) first because otherwise we can't talk about a segment at all. * the nomenclature should be clear: if we return INAT_SEG_REG_* those are own defined indices and not registers or prefixes or whatever else, so everywhere we state that we're returning an *index*. * and then shorten local variables' names as reading "reg" every other line doesn't make it clearer :) * also some comments formatting for better readability. * and prefixing register names with "r" in the comments means then all register widths, not only 32-bit. Dunno, is "(E)" SDM nomenclature for the different register widths? --- diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c index 86f58ce6c302..720529573d72 100644 --- a/arch/x86/lib/insn-eval.c +++ b/arch/x86/lib/insn-eval.c @@ -44,50 +44,45 @@ static bool is_string_insn(struct insn *insn) } /** - * get_overridden_seg_reg() - obtain segment register to use from prefixes - * @insn: Instruction structure with segment override prefixes - * @regs: Structure with register values as seen when entering kernel mode + * get_seg_reg_idx() - obtain segment register index to use from prefixes + * @insn: Instruction with segment override prefixes + * @regs: 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 running in long mode (in such a case semgment override prefixes - * are ignored. b) Whether segment override prefixes must be ignored for certain - * registers: always use CS when the register is (R|E)IP; always use ES when - * operand register is (E)DI with a string instruction as defined in the Intel - * documentation. c) If segment overrides prefixes are found in the instruction - * prefixes. d) Use the default segment register associated with the operand - * register. + * The segment register to which an effective address refers, depends on: + * + * a) whether running in long mode (in such a case segment override prefixes + * are ignored). + * + * b) Whether segment override prefixes must be ignored for certain + * registers: always use CS when the register is rIP; always use ES when + * operand register is rDI with a string instruction as defined in the Intel + * documentation. * - * This function returns the overridden segment register to use, if any, as per - * the conditions described above. Please note that this function + * c) If segment overrides prefixes are found in the instruction prefixes. + * + * d) Use the default segment register associated with the operand register. + * + * This function returns the segment register override to use, if any, + * 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 + * selector) but our defined index. The segment selector needs to be obtained + * using get_segment_selector() and passing the segment register resolved by * this function. * - * Return: A constant identifying the segment register to use, among CS, SS, DS, + * Returns: + * + * A constant identifying the segment register to use, among CS, SS, DS, * ES, FS, or GS. INAT_SEG_REG_IGNORE is returned if running in long mode. * 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. + * 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_seg_reg_idx(struct insn *insn, struct pt_regs *regs, int regoff) { - int i; - int sel_overrides = 0; - int seg_register = INAT_SEG_REG_DEFAULT; - - /* - * 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; - } + int idx = INAT_SEG_REG_DEFAULT; + int sel_overrides = 0, i; if (!insn) return -EINVAL; @@ -101,27 +96,27 @@ static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs, attr = inat_get_opcode_attribute(insn->prefixes.bytes[i]); switch (attr) { case INAT_MAKE_PREFIX(INAT_PFX_CS): - seg_register = INAT_SEG_REG_CS; + idx = INAT_SEG_REG_CS; sel_overrides++; break; case INAT_MAKE_PREFIX(INAT_PFX_SS): - seg_register = INAT_SEG_REG_SS; + idx = INAT_SEG_REG_SS; sel_overrides++; break; case INAT_MAKE_PREFIX(INAT_PFX_DS): - seg_register = INAT_SEG_REG_DS; + idx = INAT_SEG_REG_DS; sel_overrides++; break; case INAT_MAKE_PREFIX(INAT_PFX_ES): - seg_register = INAT_SEG_REG_ES; + idx = INAT_SEG_REG_ES; sel_overrides++; break; case INAT_MAKE_PREFIX(INAT_PFX_FS): - seg_register = INAT_SEG_REG_FS; + idx = INAT_SEG_REG_FS; sel_overrides++; break; case INAT_MAKE_PREFIX(INAT_PFX_GS): - seg_register = INAT_SEG_REG_GS; + idx = INAT_SEG_REG_GS; sel_overrides++; break; /* No default action needed. */ @@ -133,26 +128,26 @@ static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs, * overrides for FS and GS. */ if (user_64bit_mode(regs)) { - if (seg_register != INAT_SEG_REG_FS && - seg_register != INAT_SEG_REG_GS) + if (idx != INAT_SEG_REG_FS && + idx != INAT_SEG_REG_GS) return INAT_SEG_REG_IGNORE; /* 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. + * that use the (E)DI register. */ } else if ((regoff == offsetof(struct pt_regs, di)) && is_string_insn(insn)) { return INAT_SEG_REG_DEFAULT; } - return seg_register; + return idx; } /** - * resolve_seg_register() - obtain segment register + * resolve_seg_reg() - obtain segment register index * @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 @@ -169,36 +164,38 @@ static int get_overridden_seg_reg(struct insn *insn, struct pt_regs *regs, * * Return: A constant identifying the segment register to use, among CS, SS, DS, * ES, FS, or GS. INAT_SEG_REG_IGNORE is returned if running in long mode. + * * -EINVAL in case of error. */ -static int resolve_seg_register(struct insn *insn, struct pt_regs *regs, - int regoff) +static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff) { - int seg_reg; + int idx; - seg_reg = get_overridden_seg_reg(insn, regs, regoff); + if (!insn) + return -EINVAL; - if (seg_reg < 0) - return seg_reg; + idx = get_seg_reg_idx(insn, regs, regoff); + if (idx < 0) + return idx; - if (seg_reg == INAT_SEG_REG_IGNORE) - return seg_reg; + if (idx == INAT_SEG_REG_IGNORE) + return idx; - if (seg_reg != INAT_SEG_REG_DEFAULT) - return seg_reg; + if (idx != INAT_SEG_REG_DEFAULT) + return idx; /* * If we are here, we use the default segment register as described * in the Intel documentation: - * + DS for all references involving (E)AX, (E)CX, (E)DX, (E)BX, and - * (E)SI. - * + If used in a string instruction, ES for (E)DI. Otherwise, DS. + * + * + 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 address * 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. + * + SS for rSP or rBP. + * + CS for rIP. */ switch (regoff) { @@ -206,24 +203,26 @@ static int resolve_seg_register(struct insn *insn, struct pt_regs *regs, case offsetof(struct pt_regs, cx): case offsetof(struct pt_regs, dx): /* Need insn to verify address size. */ - if (!insn || insn->addr_bytes == 2) + 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): - /* Need insn to see if insn is string instruction. */ - if (!insn) - return -EINVAL; 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; } @@ -232,17 +231,20 @@ static int resolve_seg_register(struct insn *insn, struct pt_regs *regs, /** * get_segment_selector() - obtain segment selector * @regs: Structure with register values as seen when entering kernel mode - * @seg_reg: Segment register to use + * @seg_reg: Segment register index to use * - * Obtain the segment selector from any of the CS, SS, DS, ES, FS, GS segment - * registers. In CONFIG_X86_32, the segment is obtained from either pt_regs or - * kernel_vm86_regs as applicable. In CONFIG_X86_64, CS and SS are obtained + * Obtain the segment selector from any of the CS, SS, DS, ES, FS, GS + * segment registers. In CONFIG_X86_32, the segment is obtained from either + * pt_regs or kernel_vm86_regs as applicable. On 64-bit, CS and SS are obtained * from pt_regs. DS, ES, FS and GS are obtained by reading the actual CPU - * registers. This done for only for completeness as in CONFIG_X86_64 segment - * registers are ignored. + * registers. This done only for completeness as in long mode segment registers + * are ignored. + * + * Returns: + * + * Value of the segment selector, including null when running in long mode. * - * Return: Value of the segment selector, including null when running in - * long mode. -1 on error. + * -EINVAL on error. */ static short get_segment_selector(struct pt_regs *regs, int seg_reg) { -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) --