Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector

[Borislav Petkov <bp@suse.de>]

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)
--