| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 26 Sep 2017 12:43:53 +0200
From: Borislav Petkov <bp@...e.de>
To: Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
Cc: Ingo Molnar <mingo@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
"H. Peter Anvin" <hpa@...or.com>,
Andy Lutomirski <luto@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Brian Gerst <brgerst@...il.com>,
Chris Metcalf <cmetcalf@...lanox.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Masami Hiramatsu <mhiramat@...nel.org>,
Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
Jonathan Corbet <corbet@....net>,
"Michael S. Tsirkin" <mst@...hat.com>,
Paul Gortmaker <paul.gortmaker@...driver.com>,
Vlastimil Babka <vbabka@...e.cz>,
Chen Yucong <slaoub@...il.com>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
x86@...nel.org, ricardo.neri@...el.com,
Adam Buchbinder <adam.buchbinder@...il.com>,
Colin Ian King <colin.king@...onical.com>,
Lorenzo Stoakes <lstoakes@...il.com>,
Qiaowei Ren <qiaowei.ren@...el.com>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Adrian Hunter <adrian.hunter@...el.com>,
Kees Cook <keescook@...omium.org>,
Thomas Garnier <thgarnie@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get
segment selector
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)
--
Powered by blists - more mailing lists