Date:   Tue, 26 Sep 2017 21:21:44 -0700
From:   Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>
To:     Borislav Petkov <bp@...e.de>
Cc:     Ingo Molnar <mingo@...hat.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Andy Lutomirski <luto@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Brian Gerst <brgerst@...il.com>,
        Chris Metcalf <cmetcalf@...lanox.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Huang Rui <ray.huang@....com>, Jiri Slaby <jslaby@...e.cz>,
        Jonathan Corbet <corbet@....net>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Paul Gortmaker <paul.gortmaker@...driver.com>,
        Vlastimil Babka <vbabka@...e.cz>,
        Chen Yucong <slaoub@...il.com>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>,
        Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
        x86@...nel.org, Adam Buchbinder <adam.buchbinder@...il.com>,
        Colin Ian King <colin.king@...onical.com>,
        Lorenzo Stoakes <lstoakes@...il.com>,
        Qiaowei Ren <qiaowei.ren@...el.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Dmitry Vyukov <dvyukov@...gle.com>
Subject: Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get
 segment selector

On Tue, 2017-09-26 at 12:43 +0200, Borislav Petkov wrote:
> Hi,
> 
> On Fri, Aug 18, 2017 at 05:27:53PM -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    | 278
> > ++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 288 insertions(+)
> so I did a bunch of simplifications on top, see if you agree:
> 
> * we should always test for if (!insn) first because otherwise we
> can't talk
> about a segment at all.

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.

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.

Please see my comment below.

> 
> * the nomenclature should be clear: if we return INAT_SEG_REG_* those
> are own
> defined indices and not registers or prefixes or whatever else, so
> everywhere we
> state that we're returning an *index*.

I agree.

> 
> * and then shorten local variables' names as reading "reg" every
> other line doesn't make it clearer :)

I agree.
> 
> * also some comments formatting for better readability.

Thanks!
> 
> * and prefixing register names with "r" in the comments means then
> all
> register widths, not only 32-bit. Dunno, is "(E)" SDM nomenclature
> for
> the different register widths?

A quick look at the section 3.1.1.3 of the Intel Software Development
Manual Vol2 reveals that r/m16 operands are referred as [ACDB]X,
[SB]P,and [SD]I. r/m32 operands are referred as E[ACDB]X, E[SB]P and
E[SD]I. r/m64 operands are referred as R[ACDB]X, R[SB]P, R[SD]I and
R[8-15].

Also, some instructions (e.g., string structions) do use the
nomenclature (E)[DI]I protected mode and (R|E)[DI]I for long mode.

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.

> 
> ---
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index 86f58ce6c302..720529573d72 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -44,50 +44,45 @@ static bool is_string_insn(struct insn *insn)
>  }
>  
>  /**
> - * get_overridden_seg_reg() - obtain segment register to use from
> prefixes
> - * @insn:	Instruction structure with segment override
> prefixes
> - * @regs:	Structure with register values as seen when
> entering kernel mode
> + * get_seg_reg_idx() - obtain segment register index to use from
> prefixes
> + * @insn:	Instruction with segment override prefixes
> + * @regs:	Register values as seen when entering kernel mode
>   * @regoff:	Operand offset, in pt_regs, used to deterimine
> segment register
>   *
> - * The segment register to which an effective address refers depends
> on
> - * a) whether running in long mode (in such a case semgment override
> prefixes
> - * are ignored. b) Whether segment override prefixes must be ignored
> for certain
> - * registers: always use CS when the register is (R|E)IP; always use
> ES when
> - * operand register is (E)DI with a string instruction as defined in
> the Intel
> - * documentation. c) If segment overrides prefixes are found in the
> instruction
> - * prefixes. d) Use the default segment register associated with the
> operand
> - * register.
> + * The segment register to which an effective address refers,
> depends on:
> + *
> + * a) whether running in long mode (in such a case segment override
> prefixes
> + * are ignored).
> + *
> + * b) Whether segment override prefixes must be ignored for certain
> + * registers: always use CS when the register is rIP; always use ES
> when
> + * operand register is rDI with a string instruction as defined in
> the Intel
> + * documentation.
>   *
> - * This function returns the overridden segment register to use, if
> any, as per
> - * the conditions described above. Please note that this function
> + * c) If segment overrides prefixes are found in the instruction
> prefixes.
> + *
> + * d) Use the default segment register associated with the operand
> register.
> + *
> + * This function returns the segment register override to use, if
> any,
> + * as per the conditions described above. Please note that this
> function
>   * does not return the value in the segment register (i.e., the
> segment
> - * selector). The segment selector needs to be obtained using
> - * get_segment_selector() and passing the segment register resolved
> by
> + * selector) but our defined index. The segment selector needs to be
> obtained
> + * using get_segment_selector() and passing the segment register
> resolved by
>   * this function.
>   *
> - * Return: A constant identifying the segment register to use, among
> CS, SS, DS,
> + * Returns:
> + *
> + * A constant identifying the segment register to use, among CS, SS,
> DS,
>   * ES, FS, or GS. INAT_SEG_REG_IGNORE is returned if running in long
> mode.
>   * 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.
> + * and the default segment register shall be used.
> + *
> + * -EINVAL in case of error.
>   */

This rewording looks OK to me. Thanks!

> -static int get_overridden_seg_reg(struct insn *insn, struct pt_regs
> *regs,
> -				  int regoff)
> +static int get_seg_reg_idx(struct insn *insn, struct pt_regs *regs,
> int regoff)
>  {
> -	int i;
> -	int sel_overrides = 0;
> -	int seg_register = INAT_SEG_REG_DEFAULT;
> -
> -	/*
> -	 * 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;
> -	}

This function essentially inspects insn to find segment override
prefixes. However, if called with rIP, we still don't have any
instruction to inspect (we are yet to copy_from_user it). insn would
essentially contain garbage. I guess callers could zero-init insn in
such a case. However, I think that keeping this check makes things more
clear.

> +	int idx = INAT_SEG_REG_DEFAULT;
> +	int sel_overrides = 0, i;
>  
>  	if (!insn)
>  		return -EINVAL;
> @@ -101,27 +96,27 @@ static int get_overridden_seg_reg(struct insn
> *insn, struct pt_regs *regs,
>  		attr = inat_get_opcode_attribute(insn-
> >prefixes.bytes[i]);
>  		switch (attr) {
>  		case INAT_MAKE_PREFIX(INAT_PFX_CS):
> -			seg_register = INAT_SEG_REG_CS;
> +			idx = INAT_SEG_REG_CS;
>  			sel_overrides++;
>  			break;
>  		case INAT_MAKE_PREFIX(INAT_PFX_SS):
> -			seg_register = INAT_SEG_REG_SS;
> +			idx = INAT_SEG_REG_SS;
>  			sel_overrides++;
>  			break;
>  		case INAT_MAKE_PREFIX(INAT_PFX_DS):
> -			seg_register = INAT_SEG_REG_DS;
> +			idx = INAT_SEG_REG_DS;
>  			sel_overrides++;
>  			break;
>  		case INAT_MAKE_PREFIX(INAT_PFX_ES):
> -			seg_register = INAT_SEG_REG_ES;
> +			idx = INAT_SEG_REG_ES;
>  			sel_overrides++;
>  			break;
>  		case INAT_MAKE_PREFIX(INAT_PFX_FS):
> -			seg_register = INAT_SEG_REG_FS;
> +			idx = INAT_SEG_REG_FS;
>  			sel_overrides++;
>  			break;
>  		case INAT_MAKE_PREFIX(INAT_PFX_GS):
> -			seg_register = INAT_SEG_REG_GS;
> +			idx = INAT_SEG_REG_GS;
>  			sel_overrides++;
>  			break;
>  		/* No default action needed. */
> @@ -133,26 +128,26 @@ static int get_overridden_seg_reg(struct insn
> *insn, struct pt_regs *regs,
>  	 * overrides for FS and GS.
>  	 */
>  	if (user_64bit_mode(regs)) {
> -		if (seg_register != INAT_SEG_REG_FS &&
> -		    seg_register != INAT_SEG_REG_GS)
> +		if (idx != INAT_SEG_REG_FS &&
> +		    idx != INAT_SEG_REG_GS)
>  			return INAT_SEG_REG_IGNORE;
>  	/* 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.
> +	 * that use the (E)DI register.
>  	 */
>  	} else if ((regoff == offsetof(struct pt_regs, di)) &&
>  		   is_string_insn(insn)) {
>  		return INAT_SEG_REG_DEFAULT;
>  	}
>  
> -	return seg_register;

I will change to use indexes as you suggested.

> +	return idx;
>  }
>  
>  /**
> - * resolve_seg_register() - obtain segment register
> + * resolve_seg_reg() - obtain segment register index
>   * @insn:	Instruction structure with segment override
> prefixes
>   * @regs:	Structure with register values as seen when
> entering kernel mode
>   * @regoff:	Operand offset, in pt_regs, used to deterimine
> segment register
> @@ -169,36 +164,38 @@ static int get_overridden_seg_reg(struct insn
> *insn, struct pt_regs *regs,
>   *
>   * Return: A constant identifying the segment register to use, among
> CS, SS, DS,
>   * ES, FS, or GS. INAT_SEG_REG_IGNORE is returned if running in long
> mode.
> + *
>   * -EINVAL in case of error.
>   */
> -static int resolve_seg_register(struct insn *insn, struct pt_regs
> *regs,
> -				int regoff)
> +static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs,
> int regoff)
>  {
> -	int seg_reg;
> +	int idx;
>  
> -	seg_reg = get_overridden_seg_reg(insn, regs, regoff);
> +	if (!insn)
> +		return -EINVAL;

I checked for a NULL insn only after get_overriden_seg_reg (now
get_seg_reg_idx) because such function is able to handle a null insn.
However, this function not always needs a non-NULL insn. If obtaing the
regment register for rIP, there is not need to inspect the instruction
at all.

I only check for a NULL insn when needed (i.e., the contents of the
instruction could change the used segment register).
>  
> -	if (seg_reg < 0)
> -		return seg_reg;
> +	idx = get_seg_reg_idx(insn, regs, regoff);
> +	if (idx < 0)
> +		return idx;
>  
> -	if (seg_reg == INAT_SEG_REG_IGNORE)
> -		return seg_reg;
> +	if (idx == INAT_SEG_REG_IGNORE)
> +		return idx;
>  
> -	if (seg_reg != INAT_SEG_REG_DEFAULT)
> -		return seg_reg;
> +	if (idx != INAT_SEG_REG_DEFAULT)
> +		return idx;
>  
>  	/*
>  	 * If we are here, we use the default segment register as
> described
>  	 * in the Intel documentation:
> -	 *  + DS for all references involving (E)AX, (E)CX, (E)DX,
> (E)BX, and
> -	 * (E)SI.
> -	 *  + If used in a string instruction, ES for (E)DI.
> Otherwise, DS.
> +	 *
> +	 *  + 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 address
>  	 *    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.
> +	 *  + SS for rSP or rBP.
> +	 *  + CS for rIP.
>  	 */

Thanks for the rewording!
>  
>  	switch (regoff) {
> @@ -206,24 +203,26 @@ static int resolve_seg_register(struct insn
> *insn, struct pt_regs *regs,
>  	case offsetof(struct pt_regs, cx):
>  	case offsetof(struct pt_regs, dx):
>  		/* Need insn to verify address size. */
> -		if (!insn || insn->addr_bytes == 2)
> +		if (insn->addr_bytes == 2)

Here we care if insn is NULL as we need to look at the address size.
>  			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):
> -		/* Need insn to see if insn is string instruction.
> */
> -		if (!insn)
> -			return -EINVAL;

Here we need a valid insn to determine if it contains a string
instruction.
>  		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;

For CS we don't need insn at all.
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -232,17 +231,20 @@ static int resolve_seg_register(struct insn
> *insn, struct pt_regs *regs,
>  /**
>   * get_segment_selector() - obtain segment selector
>   * @regs:	Structure with register values as seen when
> entering kernel mode
> - * @seg_reg:	Segment register to use
> + * @seg_reg:	Segment register index to use
>   *
> - * Obtain the segment selector from any of the CS, SS, DS, ES, FS,
> GS segment
> - * registers. In CONFIG_X86_32, the segment is obtained from either
> pt_regs or
> - * kernel_vm86_regs as applicable. In CONFIG_X86_64, CS and SS are
> obtained
> + * Obtain the segment selector from any of the CS, SS, DS, ES, FS,
> GS
> + * segment registers. In CONFIG_X86_32, the segment is obtained from
> either
> + * pt_regs or kernel_vm86_regs as applicable. On 64-bit, CS and SS
> are obtained
>   * from pt_regs. DS, ES, FS and GS are obtained by reading the
> actual CPU
> - * registers. This done for only for completeness as in
> CONFIG_X86_64 segment
> - * registers are ignored.
> + * registers. This done only for completeness as in long mode
> segment registers
> + * are ignored.
> + *
> + * Returns:
> + *
> + * Value of the segment selector, including null when running in
> long mode.
>   *
> - * Return: Value of the segment selector, including null when
> running in
> - * long mode. -1 on error.
> + * -EINVAL on error.

Thanks for the rewording. I will incorporate it in the series.

Thanks and BR,
Ricardo

Powered by blists - more mailing lists