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

[Ricardo Neri]

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@xxxxxxx>
> 
> 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