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

[Ricardo Neri <ricardo.neri-calderon@linux.intel.com>]

On Wed, 2017-09-27 at 13:47 +0200, Borislav Petkov wrote:
> On Tue, Sep 26, 2017 at 09:21:44PM -0700, Ricardo Neri wrote:
> > 
> > This is true except when we don't have an insn at all (well, it may
> > be
> > non-NULL but it will only contain garbage). The case to which I am
> > referring is when we begin decoding our instruction. The first step
> > is
> > to copy_from_user the instruction and populate insn. For this we
> > must
> > calculate the linear address from where we copy using CS and rIP.
> Where do we do that?

UMIP emulation does it when evaluating if emulation is needed after a
#GP(0). It copy_from_user into insn the code at rIP that caused the
exception [1].
> 
> > 
> > Furthermore, in this only case we don't need to look at insn at all
> > as
> > the only register involved is rIP no segment override prefixes are
> > allowed.
> In any case, as it is now it sounds convoluted: you may or may not
> have an insn, and yet you call get_overridden_seg_reg() on it but you
> don't really need segment overrides because you only need CS and rIP
> initially.

The idea is that get_overridden_seg_reg() would implement the logic you
just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when
segment override prefixes are not allowed (i.e., valid insn with
operand rDI and string instruction; and rIP) or needed (i.e., long
mode, except if there are override prefixes for FS or GS); or
INAT_SEG_REG_[CSDEFG]S otherwise. 

Then resolve_seg_register() resolves the default segment if needed as
per the value returned by get_overridden_seg_reg().

Summarizing, a more accurate function name for the intended behavior is
get_overridden_seg_reg_if_any_or_needed().

> Sounds to me like this initial parsing should be done separately from
> this function...

I decided to put all the handling of segment override prefixes in a
single function.

Perhaps it could be split into two functions as follows(diff on top of
my original patches):

* Rename get_overridden_seg_reg top get_overridden_seg_reg_idx
* Remove from get_overridden_seg_reg_idx checks for rIP and rDI...
* Checks for rIP and rDI are done in a new function
* Now resolve_seg_reg calls the two functions above to determine if it
needs to resolve the default segment register index.

@@ -77,24 +77,12 @@ static bool is_string_insn(struct insn *insn)
  * 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.
  */
-static int get_overridden_seg_reg(struct insn *insn, struct pt_regs
*regs,
-				  int regoff)
+static int get_overridden_seg_reg_idx(struct insn *insn, struct
pt_regs *regs,
+				      int regoff)
 {
 	int idx = INAT_SEG_REG_DEFAULT;
 	int sel_overrides = 0, i;
 
-	/*
-	 * 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;
-	}
-
 	if (!insn)
 		return -EINVAL;
 
@@ -145,18 +133,32 @@ static int get_overridden_seg_reg(struct insn
*insn, struct pt_regs *regs,
 	/*
	 * 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.
-	 */
-	} else if ((regoff == offsetof(struct pt_regs, di)) &&
-		   is_string_insn(insn)) {
-		return INAT_SEG_REG_DEFAULT;
 	}
 
 	return idx;
 }
 
+static int use_seg_reg_overrides(struct insn *insn, int regoff)
+{
+	/*
+	 * Segment override prefixes should not be used for rIP.
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))
+		return 0;
+
+	/* Subsequent checks require a valid insn. */
+	if (!insn)
+		return -EINVAL;
+
+	if ((regoff == offsetof(struct pt_regs, di)) &&
+		   is_string_insn(insn))
+		return 0;
+
+	return 1;
+}
+
 /**
  * resolve_seg_register() - obtain segment register
  * @insn:	Instruction structure with segment override prefixes
@@ -179,22 +181,20 @@ static int get_overridden_seg_reg(struct insn
*insn, struct pt_regs *regs,
  */
 static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs,
int regoff)
 {
-	int idx;
-
-	idx = get_overridden_seg_reg(insn, regs, regoff);
+	int use_pfx_overrides;
 
-	if (idx < 0)
-		return idx;
-
-	if (idx == INAT_SEG_REG_IGNORE)
-		return idx;
+	use_pfx_overrides = use_seg_reg_overrides(insn, regoff);
+	if (use_pfx_overrides < 0)
+		return -EINVAL;
 
-	if (idx != INAT_SEG_REG_DEFAULT)
-		return idx;
+	if (use_pfx_overrides == 0)
+		goto resolve_default_idx;
 
-	if (!insn)
-		return -EINVAL;
+	return get_overridden_seg_reg_idx(insn, regs, regoff);
 
+resolve_default_idx:
+	if (user_64bit_mode(regs))
+		return INAT_SEG_REG_IGNORE;
 	/*
 	 * If we are here, we use the default segment register as 
	 * described in the Intel documentation:
@@ -209,6 +209,9 @@ static int resolve_seg_reg(struct insn *insn,
struct pt_regs *regs, int regoff)
 	 *  + CS for (E)IP.
 	 */
 
+	if (!insn)
+		return -EINVAL;
+
 	switch (regoff) {
 	case offsetof(struct pt_regs, ax):
 	case offsetof(struct pt_regs, cx):

Does this make sense?

> 
> > 
> > I only used "(E)" (i.e., not the "(R|)" part) as these utility
> > functions will deal mostly with protected mode, unless FS or GS are
> > used in long mode.
> eIP or rIP is simply much easier to type and parse. Those brackets,
> not
> really.

Agreed. Then I will use rIP.
> 
> > 
> > I only check for a NULL insn when needed (i.e., the contents of the
> > instruction could change the used segment register).
> ... and those if (!insn) tests sprinkled around simply make the code
> unreadable and if we can get rid of them, we should.

Sure, you are correct this will make code more readable.

Thanks and BR,
Ricardo

[1]. https://github.com/ricardon/tip/blob/rneri/umip_v9/arch/x86/kernel
/umip.c#L276