On Wed, 2017-10-11 at 00:41 +0200, Borislav Petkov wrote: > On Tue, Oct 03, 2017 at 08:54:16PM -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 | 321 > > ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 331 insertions(+) > Ok, some more fixes ontop. I carved out the code under the > resolve_default_idx: label into a separate function. This made > resolve_seg_reg() pretty-much trivial to follow. Also renamed some > functions and variables to better denote what they do.
Thanks! I will take your changes. > > Please add > > Improvements-by: Borislav Petkov <[email protected]> > > to your commit message if you use this. Thanks. Will do. > > --- > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c > index 77b48f99d73a..d02b94ace0f1 100644 > --- a/arch/x86/lib/insn-eval.c > +++ b/arch/x86/lib/insn-eval.c > @@ -49,7 +49,7 @@ static bool is_string_insn(struct insn *insn) > } > > /** > - * get_overridden_seg_reg_idx() - obtain segment register override index > + * get_seg_reg_override_idx() - obtain segment register override index > * @insn: Instruction with segment override prefixes > * > * Inspect the instruction prefixes and find segment overrides, if any. > @@ -62,10 +62,10 @@ static bool is_string_insn(struct insn *insn) > * > * -EINVAL in case of error. > */ > -static int get_overridden_seg_reg_idx(struct insn *insn) > +static int get_seg_reg_override_idx(struct insn *insn) > { > int idx = INAT_SEG_REG_DEFAULT; > - int sel_overrides = 0, i; > + int num_overrides = 0, i; > > if (!insn) > return -EINVAL; > @@ -80,41 +80,41 @@ static int get_overridden_seg_reg_idx(struct insn *insn) > switch (attr) { > case INAT_MAKE_PREFIX(INAT_PFX_CS): > idx = INAT_SEG_REG_CS; > - sel_overrides++; > + num_overrides++; > break; > case INAT_MAKE_PREFIX(INAT_PFX_SS): > idx = INAT_SEG_REG_SS; > - sel_overrides++; > + num_overrides++; > break; > case INAT_MAKE_PREFIX(INAT_PFX_DS): > idx = INAT_SEG_REG_DS; > - sel_overrides++; > + num_overrides++; > break; > case INAT_MAKE_PREFIX(INAT_PFX_ES): > idx = INAT_SEG_REG_ES; > - sel_overrides++; > + num_overrides++; > break; > case INAT_MAKE_PREFIX(INAT_PFX_FS): > idx = INAT_SEG_REG_FS; > - sel_overrides++; > + num_overrides++; > break; > case INAT_MAKE_PREFIX(INAT_PFX_GS): > idx = INAT_SEG_REG_GS; > - sel_overrides++; > + num_overrides++; > break; > /* No default action needed. */ > } > } > > /* More than one segment override prefix leads to undefined behavior. > */ > - if (sel_overrides > 1) > + if (num_overrides > 1) > return -EINVAL; > > return idx; > } > > /** > - * allow_seg_reg_overrides() - check if segment override prefixes are allowed > + * check_seg_overrides() - check if segment override prefixes are allowed > * @insn: Instruction with segment override prefixes > * @regoff: Operand offset, in pt_regs, for which the check is > performed > * > @@ -129,7 +129,7 @@ static int get_overridden_seg_reg_idx(struct insn *insn) > * > * -EINVAL in case of error. > */ > -static int allow_seg_reg_overrides(struct insn *insn, int regoff) > +static int check_seg_overrides(struct insn *insn, int regoff) > { > /* > * Segment override prefixes should not be used for rIP. It is not > @@ -148,6 +148,55 @@ static int allow_seg_reg_overrides(struct insn *insn, int > regoff) > return 1; > } > > +static int resolve_default_seg(struct insn *insn, struct pt_regs *regs, int > off) > +{ Shouldn't this function check for a null insn since it is used here? > + 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: > + * > + * + 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 > addresses. > + * 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. > + */ > + switch (off) { > + case offsetof(struct pt_regs, ax): > + case offsetof(struct pt_regs, cx): > + case offsetof(struct pt_regs, dx): > + /* Need insn to verify address size. */ > + 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): > + 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; > + } > +} > + > + > /** > * resolve_seg_reg() - obtain segment register index > * @insn: Instruction with operands > @@ -194,24 +243,24 @@ static int allow_seg_reg_overrides(struct insn *insn, > int regoff) > */ > static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs, int > regoff) > { > - int use_pfx_overrides, idx; > + int ret, idx; > > - use_pfx_overrides = allow_seg_reg_overrides(insn, regoff); > - if (use_pfx_overrides < 0) > - return use_pfx_overrides; > + ret = check_seg_overrides(insn, regoff); > + if (ret < 0) > + return ret; > > - if (use_pfx_overrides == 0) > - goto resolve_default_idx; > + if (!ret) > + return resolve_default_seg(insn, regs, regoff); > > if (!insn) > return -EINVAL; Could this check be removed? insn is not used for anything but passed to other functions that do perform this check. Thanks and BR, Ricardo
