| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 11 Oct 2017 18:12:30 -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 v9 13/29] x86/insn-eval: Add utility functions to get
segment selector
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@...e.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
Powered by blists - more mailing lists