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
