Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1739779 > unrolled thread
| Started by | Borislav Petkov <bp@suse.de> |
|---|---|
| First post | 2017-09-26 12:50 +0200 |
| Last post | 2017-09-29 14:00 +0200 |
| Articles | 7 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Borislav Petkov <bp@suse.de> - 2017-09-26 12:50 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Ricardo Neri <ricardo.neri-calderon@linux.intel.com> - 2017-09-27 06:30 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Borislav Petkov <bp@suse.de> - 2017-09-27 13:50 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Ricardo Neri <ricardo.neri-calderon@linux.intel.com> - 2017-09-28 00:40 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Borislav Petkov <bp@suse.de> - 2017-09-28 11:40 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Ricardo Neri <ricardo.neri-calderon@linux.intel.com> - 2017-09-29 08:10 +0200
Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector Borislav Petkov <bp@suse.de> - 2017-09-29 14:00 +0200
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-09-26 12:50 +0200 |
| Subject | Re: [PATCH v8 12/28] x86/insn-eval: Add utility functions to get segment selector |
| Message-ID | <utVB7-3Ca-11@gated-at.bofh.it> |
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.
* 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*.
* and then shorten local variables' names as reading "reg" every
other line doesn't make it clearer :)
* also some comments formatting for better readability.
* 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?
---
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.
*/
-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;
- }
+ 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;
+ 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;
- 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.
*/
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)
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;
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;
}
@@ -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.
*/
static short get_segment_selector(struct pt_regs *regs, int seg_reg)
{
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [next] | [standalone]
| From | Ricardo Neri <ricardo.neri-calderon@linux.intel.com> |
|---|---|
| Date | 2017-09-27 06:30 +0200 |
| Message-ID | <uuc8V-5Vj-1@gated-at.bofh.it> |
| In reply to | #1739779 |
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
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-09-27 13:50 +0200 |
| Message-ID | <uuj0K-1P9-5@gated-at.bofh.it> |
| In reply to | #1740382 |
On Tue, Sep 26, 2017 at 09:21:44PM -0700, Ricardo Neri wrote:
> 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.
Where do we do that?
> 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.
In any case, as it is now it sounds convoluted: you may or may not
have an insn, and yet you call get_overridden_seg_reg() on it but you
don't really need segment overrides because you only need CS and rIP
initially.
Sounds to me like this initial parsing should be done separately from
this function...
> 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.
eIP or rIP is simply much easier to type and parse. Those brackets, not
really.
> I only check for a NULL insn when needed (i.e., the contents of the
> instruction could change the used segment register).
... and those if (!insn) tests sprinkled around simply make the code
unreadable and if we can get rid of them, we should.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Ricardo Neri <ricardo.neri-calderon@linux.intel.com> |
|---|---|
| Date | 2017-09-28 00:40 +0200 |
| Message-ID | <uut9M-AW-11@gated-at.bofh.it> |
| In reply to | #1740635 |
On Wed, 2017-09-27 at 13:47 +0200, Borislav Petkov wrote:
> On Tue, Sep 26, 2017 at 09:21:44PM -0700, Ricardo Neri wrote:
> >
> > 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.
> Where do we do that?
UMIP emulation does it when evaluating if emulation is needed after a
#GP(0). It copy_from_user into insn the code at rIP that caused the
exception [1].
>
> >
> > 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.
> In any case, as it is now it sounds convoluted: you may or may not
> have an insn, and yet you call get_overridden_seg_reg() on it but you
> don't really need segment overrides because you only need CS and rIP
> initially.
The idea is that get_overridden_seg_reg() would implement the logic you
just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when
segment override prefixes are not allowed (i.e., valid insn with
operand rDI and string instruction; and rIP) or needed (i.e., long
mode, except if there are override prefixes for FS or GS); or
INAT_SEG_REG_[CSDEFG]S otherwise.
Then resolve_seg_register() resolves the default segment if needed as
per the value returned by get_overridden_seg_reg().
Summarizing, a more accurate function name for the intended behavior is
get_overridden_seg_reg_if_any_or_needed().
> Sounds to me like this initial parsing should be done separately from
> this function...
I decided to put all the handling of segment override prefixes in a
single function.
Perhaps it could be split into two functions as follows(diff on top of
my original patches):
* Rename get_overridden_seg_reg top get_overridden_seg_reg_idx
* Remove from get_overridden_seg_reg_idx checks for rIP and rDI...
* Checks for rIP and rDI are done in a new function
* Now resolve_seg_reg calls the two functions above to determine if it
needs to resolve the default segment register index.
@@ -77,24 +77,12 @@ static bool is_string_insn(struct insn *insn)
* 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.
*/
-static int get_overridden_seg_reg(struct insn *insn, struct pt_regs
*regs,
- int regoff)
+static int get_overridden_seg_reg_idx(struct insn *insn, struct
pt_regs *regs,
+ int regoff)
{
int idx = INAT_SEG_REG_DEFAULT;
int sel_overrides = 0, i;
- /*
- * 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;
- }
-
if (!insn)
return -EINVAL;
@@ -145,18 +133,32 @@ static int get_overridden_seg_reg(struct insn
*insn, struct pt_regs *regs,
/*
* 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.
- */
- } else if ((regoff == offsetof(struct pt_regs, di)) &&
- is_string_insn(insn)) {
- return INAT_SEG_REG_DEFAULT;
}
return idx;
}
+static int use_seg_reg_overrides(struct insn *insn, int regoff)
+{
+ /*
+ * Segment override prefixes should not be used for rIP.
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))
+ return 0;
+
+ /* Subsequent checks require a valid insn. */
+ if (!insn)
+ return -EINVAL;
+
+ if ((regoff == offsetof(struct pt_regs, di)) &&
+ is_string_insn(insn))
+ return 0;
+
+ return 1;
+}
+
/**
* resolve_seg_register() - obtain segment register
* @insn: Instruction structure with segment override prefixes
@@ -179,22 +181,20 @@ static int get_overridden_seg_reg(struct insn
*insn, struct pt_regs *regs,
*/
static int resolve_seg_reg(struct insn *insn, struct pt_regs *regs,
int regoff)
{
- int idx;
-
- idx = get_overridden_seg_reg(insn, regs, regoff);
+ int use_pfx_overrides;
- if (idx < 0)
- return idx;
-
- if (idx == INAT_SEG_REG_IGNORE)
- return idx;
+ use_pfx_overrides = use_seg_reg_overrides(insn, regoff);
+ if (use_pfx_overrides < 0)
+ return -EINVAL;
- if (idx != INAT_SEG_REG_DEFAULT)
- return idx;
+ if (use_pfx_overrides == 0)
+ goto resolve_default_idx;
- if (!insn)
- return -EINVAL;
+ return get_overridden_seg_reg_idx(insn, regs, regoff);
+resolve_default_idx:
+ 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:
@@ -209,6 +209,9 @@ static int resolve_seg_reg(struct insn *insn,
struct pt_regs *regs, int regoff)
* + CS for (E)IP.
*/
+ if (!insn)
+ return -EINVAL;
+
switch (regoff) {
case offsetof(struct pt_regs, ax):
case offsetof(struct pt_regs, cx):
Does this make sense?
>
> >
> > 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.
> eIP or rIP is simply much easier to type and parse. Those brackets,
> not
> really.
Agreed. Then I will use rIP.
>
> >
> > I only check for a NULL insn when needed (i.e., the contents of the
> > instruction could change the used segment register).
> ... and those if (!insn) tests sprinkled around simply make the code
> unreadable and if we can get rid of them, we should.
Sure, you are correct this will make code more readable.
Thanks and BR,
Ricardo
[1]. https://github.com/ricardon/tip/blob/rneri/umip_v9/arch/x86/kernel
/umip.c#L276
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-09-28 11:40 +0200 |
| Message-ID | <uuDsv-7h4-15@gated-at.bofh.it> |
| In reply to | #1741076 |
On Wed, Sep 27, 2017 at 03:32:26PM -0700, Ricardo Neri wrote:
> The idea is that get_overridden_seg_reg() would implement the logic you
> just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when
> segment override prefixes are not allowed (i.e., valid insn with
> operand rDI and string instruction; and rIP) or needed (i.e., long
> mode, except if there are override prefixes for FS or GS); or
> INAT_SEG_REG_[CSDEFG]S otherwise.
Ok, lemme see if we're talking the same thing. Your diff is linewrapped
so parsing that is hard.
Do this
if (regoff == offsetof(struct pt_regs, ip)) {
if (user_64bit_mode(regs))
return INAT_SEG_REG_IGNORE;
else
return INAT_SEG_REG_DEFAULT;
}
and all the other checking *before* you do insn_init(). Because you have
crazy stuff like:
if (seg_reg == INAT_SEG_REG_IGNORE)
return seg_reg;
which shortcuts those functions and is simply clumsy and complicates
following the code. The mere fact that you have to call the function
"get_overridden_seg_reg_if_any_or_needed()" already tells you that that
function is doing too many things at once.
When the function is called get_segment_register() then it should do
only that. And all the checking is done before or in wrappers.
IOW, all the rIP checking and early return down the
insn_get_seg_base() -> resolve_seg_register() -> .. should be done
separately.
*Then* you do insn_init() and hand it down to insn_get_seg_base() and
from now on you have a proper insn pointer which you hand around and
check for NULL only once, on function entry.
Then your code flow is much simpler: first you take care of the case
where rIP doesn't do segment overrides and all the other cases are
handled by the normal path, with a proper struct insn.
Makes more sense?
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [next] | [standalone]
| From | Ricardo Neri <ricardo.neri-calderon@linux.intel.com> |
|---|---|
| Date | 2017-09-29 08:10 +0200 |
| Message-ID | <uuWEN-2sN-5@gated-at.bofh.it> |
| In reply to | #1741334 |
On Thu, 2017-09-28 at 11:36 +0200, Borislav Petkov wrote:
> On Wed, Sep 27, 2017 at 03:32:26PM -0700, Ricardo Neri wrote:
> >
> > The idea is that get_overridden_seg_reg() would implement the logic you
> > just described. It would return return INAT_SEG_REG_DEFAULT/IGNORE when
> > segment override prefixes are not allowed (i.e., valid insn with
> > operand rDI and string instruction; and rIP) or needed (i.e., long
> > mode, except if there are override prefixes for FS or GS); or
> > INAT_SEG_REG_[CSDEFG]S otherwise.
> Ok, lemme see if we're talking the same thing. Your diff is linewrapped
> so parsing that is hard.
>
> Do this
>
> if (regoff == offsetof(struct pt_regs, ip)) {
> if (user_64bit_mode(regs))
> return INAT_SEG_REG_IGNORE;
> else
> return INAT_SEG_REG_DEFAULT;
> }
>
> and all the other checking *before* you do insn_init(). Because you have
> crazy stuff like:
>
> if (seg_reg == INAT_SEG_REG_IGNORE)
> return seg_reg;
>
> which shortcuts those functions and is simply clumsy and complicates
> following the code. The mere fact that you have to call the function
> "get_overridden_seg_reg_if_any_or_needed()" already tells you that that
> function is doing too many things at once.
>
> When the function is called get_segment_register() then it should do
> only that. And all the checking is done before or in wrappers.
Yes, I realized this while I was typing.
>
> IOW, all the rIP checking and early return down the
> insn_get_seg_base() -> resolve_seg_register() -> .. should be done
> separately.
Agreed now.
>
> *Then* you do insn_init() and hand it down to insn_get_seg_base() and
> from now on you have a proper insn pointer which you hand around and
> check for NULL only once, on function entry.
I agree. In fact, insn_get_seg_base() does not need insn at all. All it needs is
a INAT_SEG_REG_* index. This would make things clear. UMIP (and callers that
need to copy_from_user code can do insn_get_seg_base(regs, INAT_SEG_REG_CS). No
insn needed.
In fact, it is only the insn_get_addr_ref_xx() family of functions that does
need to inspect insn (which will be populated and valided) to determine the what
registers are used as operands... and determine the applicable segment register.
However, insn_get_addr_ref_xx() functions call insn_get_seg_base() several times
each. Each time they would need to do:
if (can_use_seg_override_prefixes(insn, regoff))
idx = get_overriden_seg_reg(insn, regs)
else
idx = get_default_seg_reg()
The pseudocode above looks like a resolve_reg_idx() to me.
Then insn_get_addr_ref_xx() can call insn_get_seg_base(idx).
>
> Then your code flow is much simpler: first you take care of the case
> where rIP doesn't do segment overrides and all the other cases are
> handled by the normal path, with a proper struct insn.
Do you think the pseudocode above addresses your concerns?
*insn_get_seg_base() will take a INAT_SEG_REG_* index
*insn_get_ref_xx() receives an initialized insn that can check for NULL value.
*a reworked resolve_seg_reg_idx will clearly check if it can use segment
override prefixes and obtain them. If not, it will use default values.
Thanks and BR,
Ricardo
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@suse.de> |
|---|---|
| Date | 2017-09-29 14:00 +0200 |
| Message-ID | <uv27w-5HZ-5@gated-at.bofh.it> |
| In reply to | #1741932 |
On Thu, Sep 28, 2017 at 11:06:42PM -0700, Ricardo Neri wrote:
> I agree. In fact, insn_get_seg_base() does not need insn at all. All it needs is
> a INAT_SEG_REG_* index. This would make things clear. UMIP (and callers that
> need to copy_from_user code can do insn_get_seg_base(regs, INAT_SEG_REG_CS). No
> insn needed.
Yap.
> In fact, it is only the insn_get_addr_ref_xx() family of functions that does
I think you mean get_addr_ref_xx() here.
> Do you think the pseudocode above addresses your concerns?
>
> *insn_get_seg_base() will take a INAT_SEG_REG_* index
> *insn_get_ref_xx() receives an initialized insn that can check for NULL value.
> *a reworked resolve_seg_reg_idx will clearly check if it can use segment
> override prefixes and obtain them. If not, it will use default values.
Makes sense, but send me the final version to take a look at it too.
Thanks.
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
--
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web