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

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

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