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

Ricardo Neri
2017-10-12T01:13:05Z

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 <bp@suse.de>
>
> 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