Re: [PATCH v9 13/29] x86/insn-eval: Add utility functions to get segment selector

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

On Fri, 2017-10-13 at 13:37 +0200, Borislav Petkov wrote:
> On Thu, Oct 12, 2017 at 06:08:17PM -0700, Ricardo Neri wrote:
> > 
> > In my opinion it would be better to have all the checks in a single place.
> > This
> > makes the code easier to read that having this special case directly
> > in resolve_default_seg().
> > ...
> > Rather than checking for null insn in resolve_seg_reg(), which does not use
> > it,
> > let the functions it calls do the check if they need to.
> Of course it is using it - it is passing it down to callers.
> 
> No, this is completely backwards. You're pushing the if (!insn) check
> down instead of up. What you wanna do instead is get that "strange" case
> out of the way *first* where insn is NULL and then have the remaining
> flow with a properly allocated struct insn.

Furthermore, resolve_seg_reg() there should not be need to call
resolve_seg_reg(). It is meant to be used when decoding instructions. Callers
already know they must use CS for rIP. resolve_seg_reg() is to be used when the
segment register is not know beforehand.

Then it sounds that your second proposal address it:

1) It first handles the strange case
2) It checks if insn is valid
3) Checks if segment overrides prefixes can be used
   3.a) If yes, use them, if any.
   3.b) If no, resolve default segment register index.

resolve_default_seg() and get_seg_reg_override_idx() will assume that insn is
not null. Please find at the bottom a diff of your second proposal with minor
tweaks.

> 
> And the only case where insn is NULL is fixup_umip_exception(). All the
> other callers of insn_get_seg_base() supply a properly setup struct insn
> * AFAICT.

Yes, this is correct.
> 
> So do the minimum work of getting the segment base either directly in
> fixup_umip_exception() by calling a helper function as it matters only
> there.
> 
> And IINM, you have two possible cases:
> 
> 1. INAT_SEG_REG_IGNORE which makes segment base 0
> 
> 2. INAT_SEG_REG_DEFAULT which maps to INAT_SEG_REG_CS for the rIP case
> and then gets the selector:
> 
> 	sel = (unsigned short)(regs->cs & 0xffff);
> and then computes the base.

Yes, also correct; I modified your second proposal for cases 1 and 2.

> 
> And the mapping of sel to base you can do by carving out the piece of
> insn_get_seg_base() *after* you've computed @sel and you do the base
> computation, i.e., the piece which starts with this:
> 
>         if (v8086_mode(regs))
>                 /*
>                  * Base is simply the segment selector shifted 4
>                  * positions to the right.
> 	...
> 
> into a separate function called __get_seg_base(sel, ...).
> 
> The important thing to note here is that this function won't need insn
> so you can call it without one.

In v9 insn_get_seg_base() does not take an insn as argument (it used to take one
until v8). Also, fixup_umip_exception() calls insn_get_seg_base with
INAT_SEG_REG_CS, no need to resolve the segment register. It does it only for
!user_64bit_mode().

If a __get_seg_base(sel, ...) was implemented, then insn_get_seg_base() would
look like:

unsigned long insn_get_seg_base(struct pt_regs *regs, int seg_reg_idx)
{
	struct desc_struct *desc;
	short sel;

	sel = get_segment_selector(regs, seg_reg_idx);
	if (sel < 0)
		return -1L;
		}

	return __get_seg_base(sel, regs);
}

After having clarified that an insn is not needed in the latest iteration of
insn_get_seg_base(), I just want to double check if this is what you would like
to see.

> 
> This way you have it nice and clean designed with a clear separation of
> the cases *before* a valid struct insn * and *after*.
> 
> Right now, you have both intermixed and the code is hard to follow as
> you have to pay attention at each time: how and where am I being called.
> 
> Makes sense?

I think it does now. This is a modification of your second proposal (I hope text
does not wrap):

@@ -71,9 +71,6 @@ static int get_seg_reg_override_idx(struct insn *insn)
 	int idx = INAT_SEG_REG_DEFAULT;
 	int num_overrides = 0, i;
 
-	if (!insn)
-		return -EINVAL;
-
 	insn_get_prefixes(insn);
 
 	/* Look for any segment override prefixes. */
@@ -128,28 +125,16 @@ static int get_seg_reg_override_idx(struct insn *insn)
  *
  * Returns:
  *
- * 1 if segment override prefixes can be used with the register indicated
- * in @regoff. 0 if otherwise.
+ * True if segment override prefixes can be used with the register indicated
+ * in @regoff. False if otherwise.
  *
- * -EINVAL in case of error.
  */
-static int check_seg_overrides(struct insn *insn, int regoff)
+static bool check_seg_overrides(struct insn *insn, int regoff)
 {
-	/*
-	 * Segment override prefixes should not be used for rIP. It is not
-	 * necessary to inspect the instruction structure.
-	 */
-	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 false;
 
-	return 1;
+	return true;
 }
 
 /**
@@ -249,18 +234,26 @@ static int resolve_default_seg(struct insn *insn, struct
pt_regs *regs, int off)
  */
 static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int regoff)
 {
-	int ret, idx;
-
-	ret = check_seg_overrides(insn, regoff);
-	if (ret < 0)
-		return ret;
+	int idx;
 
-	if (!ret)
-		return resolve_default_seg(insn, regs, regoff);
+	/*
+	 * In the unlikely event of having to resolve the segment register
+	 * index for rIP, do it first. Segment override prefixes should not
+	 * be used. Hence, it is not necessary to inspect the instruction.
+	 */
+	if (regoff == offsetof(struct pt_regs, ip)) {
+		if (user_64bit_mode(regs))
+			return INAT_SEG_REG_IGNORE;
+		else
+			return INAT_SEG_REG_CS;
+	}
 
 	if (!insn)
 		return -EINVAL;
 
+	if (!check_seg_overrides(insn, regoff))
+		return resolve_default_seg(insn, regs, regoff);
+
 	idx = get_seg_reg_override_idx(insn);
 	if (idx < 0)
 		return idx;
Thanks and BR,
Ricardo