Messages in this thread Patch in this message | | | Subject | Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector | From | Ricardo Neri <> | Date | Wed, 27 Sep 2017 15:32:26 -0700 |
| |
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
| |