Re: [v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero
On Sun, 2017-05-07 at 19:20 +0200, Borislav Petkov wrote: > On Wed, Apr 26, 2017 at 06:29:59PM -0700, Ricardo Neri wrote: > > > if (X86_MODRM_MOD(insn->modrm.value) == 0 && > > > X86_MODRM_RM(insn->modrm.value) == 5) > > > > > > looks more understandable to me. > > > > Should I go with !(X86_MODRM_MOD(insn->modrm.value)) as you suggested in > > other patches? > > Ah, yes pls. > I did this in v7[1]. Thanks and BR, Ricardo [1]. https://lkml.org/lkml/2017/5/5/399 -- To unsubscribe from this list: send the line "unsubscribe linux-msdos" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero
On Wed, Apr 26, 2017 at 06:29:59PM -0700, Ricardo Neri wrote: > > if (X86_MODRM_MOD(insn->modrm.value) == 0 && > > X86_MODRM_RM(insn->modrm.value) == 5) > > > > looks more understandable to me. > > Should I go with !(X86_MODRM_MOD(insn->modrm.value)) as you suggested in > other patches? Ah, yes pls. Thanks. -- Regards/Gruss, Boris. SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg) -- -- To unsubscribe from this list: send the line "unsubscribe linux-msdos" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero
On Fri, 2017-04-21 at 12:52 +0200, Borislav Petkov wrote:
> On Tue, Mar 07, 2017 at 04:32:43PM -0800, Ricardo Neri wrote:
> > Section 2.2.1.3 of the Intel 64 and IA-32 Architectures Software
> > Developer's Manual volume 2A states that when the mod part of the ModRM
> > byte is zero and R/EBP is specified in the R/M part of such bit, the value
> > of the aforementioned register should not be used in the address
> > computation. Instead, a 32-bit displacement is expected. The instruction
> > decoder takes care of setting the displacement to the expected value.
> > Returning -EDOM signals callers that they should ignore the value of such
> > register when computing the address encoded in the instruction operands.
> >
> > Also, callers should exercise care to correctly interpret this particular
> > case. In IA-32e 64-bit mode, the address is given by the displacement plus
> > the value of the RIP. In IA-32e compatibility mode, the value of EIP is
> > ignored. This correction is done for our insn_get_addr_ref.
> >
> > Cc: Dave Hansen
> > Cc: Adam Buchbinder
> > Cc: Colin Ian King
> > Cc: Lorenzo Stoakes
> > Cc: Qiaowei Ren
> > Cc: Arnaldo Carvalho de Melo
> > Cc: Masami Hiramatsu
> > Cc: Adrian Hunter
> > Cc: Kees Cook
> > Cc: Thomas Garnier
> > Cc: Peter Zijlstra
> > Cc: Borislav Petkov
> > Cc: Dmitry Vyukov
> > Cc: Ravi V. Shankar
> > Cc: x...@kernel.org
> > Signed-off-by: Ricardo Neri
> > ---
> > arch/x86/lib/insn-eval.c | 25 +++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> > index cda6c71..ea10b03 100644
> > --- a/arch/x86/lib/insn-eval.c
> > +++ b/arch/x86/lib/insn-eval.c
> > @@ -250,6 +250,14 @@ static int get_reg_offset(struct insn *insn, struct
> > pt_regs *regs,
> > switch (type) {
> > case REG_TYPE_RM:
> > regno = X86_MODRM_RM(insn->modrm.value);
> > + /* if mod=0, register R/EBP is not used in the address
> > +* computation. Instead, a 32-bit displacement is expected;
> > +* the instruction decoder takes care of reading such
> > +* displacement. This is true for both R/EBP and R13, as the
> > +* REX.B bit is not decoded.
> > +*/
>
> I'd simply write here: "ModRM.mod == 0 and ModRM.rm == 5 means a 32-bit
> displacement is following."
I will shorten the comment.
>
> In addition, kernel comments style is:
>
> /*
>* A sentence ending with a full-stop.
>* Another sentence. ...
>* More sentences. ...
>*/
... and use the correct style. I feel bad I missed this one.
>
> > + if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) == 0)
> > + return -EDOM;
>
> if (X86_MODRM_MOD(insn->modrm.value) == 0 &&
> X86_MODRM_RM(insn->modrm.value) == 5)
>
> looks more understandable to me.
Should I go with !(X86_MODRM_MOD(insn->modrm.value)) as you suggested in
other patches?
>
> > if (X86_REX_B(insn->rex_prefix.value))
> > regno += 8;
> > break;
> > @@ -599,9 +607,22 @@ void __user *insn_get_addr_ref(struct insn *insn,
> > struct pt_regs *regs)
> > eff_addr = base + indx * (1 << X86_SIB_SCALE(sib));
> > } else {
> > addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> > - if (addr_offset < 0)
> > + /* -EDOM means that we must ignore the address_offset.
> > +* The only case in which we see this value is when
> > +* R/M points to R/EBP. In such a case, in 64-bit mode
> > +* the effective address is relative to tho RIP.
>
> s/tho//
Will correct.
>
> > +*/
>
> Kernel comments style is:
>
> /*
>* A sentence ending with a full-stop.
>* Another sentence. ...
>* More sentences. ...
>*/
>
Will correct.
> > + if (addr_offset == -EDOM) {
> > + eff_addr = 0;
> > +#ifdef CONFIG_X86_64
> > + if (user_64bit_mode(regs))
> > + eff_addr = (long)regs->ip;
>
> Is regs->ip the rIP of the *following* insn?
No this is a bug. This should be regs->ip + insn.length.
>
> > +#endif
>
> You can do this in a prepatch and then get rid of the ifdeffery here:
>
> diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
> index 2b5d686ea9f3..f6239273c5f1 100644
> --- a/arch/x86/include/asm/ptrace.h
> +++ b/arch/x86/include/asm/ptrace.h
> @@ -115,9 +115,9 @@ static inline int v8086_mode(struct pt_regs *regs)
> #endif
> }
>
> -#ifdef CONFIG_X86_64
> static inline bool user_64bit_mode(struct pt_regs *regs)
> {
> +#ifdef CONFIG_X86_64
> #ifndef CONFIG_PARAVIRT
> /*
>* On non-paravirt systems, this is the only long mode CPL 3
> @@ -128,6 +128,9 @@ static
Re: [v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero
On Tue, Mar 07, 2017 at 04:32:43PM -0800, Ricardo Neri wrote:
> Section 2.2.1.3 of the Intel 64 and IA-32 Architectures Software
> Developer's Manual volume 2A states that when the mod part of the ModRM
> byte is zero and R/EBP is specified in the R/M part of such bit, the value
> of the aforementioned register should not be used in the address
> computation. Instead, a 32-bit displacement is expected. The instruction
> decoder takes care of setting the displacement to the expected value.
> Returning -EDOM signals callers that they should ignore the value of such
> register when computing the address encoded in the instruction operands.
>
> Also, callers should exercise care to correctly interpret this particular
> case. In IA-32e 64-bit mode, the address is given by the displacement plus
> the value of the RIP. In IA-32e compatibility mode, the value of EIP is
> ignored. This correction is done for our insn_get_addr_ref.
>
> Cc: Dave Hansen
> Cc: Adam Buchbinder
> Cc: Colin Ian King
> Cc: Lorenzo Stoakes
> Cc: Qiaowei Ren
> Cc: Arnaldo Carvalho de Melo
> Cc: Masami Hiramatsu
> Cc: Adrian Hunter
> Cc: Kees Cook
> Cc: Thomas Garnier
> Cc: Peter Zijlstra
> Cc: Borislav Petkov
> Cc: Dmitry Vyukov
> Cc: Ravi V. Shankar
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri
> ---
> arch/x86/lib/insn-eval.c | 25 +++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
> index cda6c71..ea10b03 100644
> --- a/arch/x86/lib/insn-eval.c
> +++ b/arch/x86/lib/insn-eval.c
> @@ -250,6 +250,14 @@ static int get_reg_offset(struct insn *insn, struct
> pt_regs *regs,
> switch (type) {
> case REG_TYPE_RM:
> regno = X86_MODRM_RM(insn->modrm.value);
> + /* if mod=0, register R/EBP is not used in the address
> + * computation. Instead, a 32-bit displacement is expected;
> + * the instruction decoder takes care of reading such
> + * displacement. This is true for both R/EBP and R13, as the
> + * REX.B bit is not decoded.
> + */
I'd simply write here: "ModRM.mod == 0 and ModRM.rm == 5 means a 32-bit
displacement is following."
In addition, kernel comments style is:
/*
* A sentence ending with a full-stop.
* Another sentence. ...
* More sentences. ...
*/
> + if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) == 0)
> + return -EDOM;
if (X86_MODRM_MOD(insn->modrm.value) == 0 &&
X86_MODRM_RM(insn->modrm.value) == 5)
looks more understandable to me.
> if (X86_REX_B(insn->rex_prefix.value))
> regno += 8;
> break;
> @@ -599,9 +607,22 @@ void __user *insn_get_addr_ref(struct insn *insn, struct
> pt_regs *regs)
> eff_addr = base + indx * (1 << X86_SIB_SCALE(sib));
> } else {
> addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> - if (addr_offset < 0)
> + /* -EDOM means that we must ignore the address_offset.
> + * The only case in which we see this value is when
> + * R/M points to R/EBP. In such a case, in 64-bit mode
> + * the effective address is relative to tho RIP.
s/tho//
> + */
Kernel comments style is:
/*
* A sentence ending with a full-stop.
* Another sentence. ...
* More sentences. ...
*/
> + if (addr_offset == -EDOM) {
> + eff_addr = 0;
> +#ifdef CONFIG_X86_64
> + if (user_64bit_mode(regs))
> + eff_addr = (long)regs->ip;
Is regs->ip the rIP of the *following* insn?
> +#endif
You can do this in a prepatch and then get rid of the ifdeffery here:
diff --git a/arch/x86/include/asm/ptrace.h b/arch/x86/include/asm/ptrace.h
index 2b5d686ea9f3..f6239273c5f1 100644
--- a/arch/x86/include/asm/ptrace.h
+++ b/arch/x86/include/asm/ptrace.h
@@ -115,9 +115,9 @@ static inline int v8086_mode(struct pt_regs *regs)
#endif
}
-#ifdef CONFIG_X86_64
static inline bool user_64bit_mode(struct pt_regs *regs)
{
+#ifdef CONFIG_X86_64
#ifndef CONFIG_PARAVIRT
/*
* On non-paravirt systems, this is the only long mode CPL 3
@@ -128,6 +128,9 @@ static inline bool user_64bit_mode(struct pt_regs *regs)
/* Headers are too twisted for this to go in paravirt.h. */
return regs->cs == __USER_CS || regs->cs == pv_info.extra_user_64bit_cs;
#endif
+#else /* !CONFIG_X86_64 */
+ return false;
+#endif
}
#define current_user_stack_pointer() current_pt_regs()->sp
---
--
Regards/Gruss,
Boris.
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284
(AG Nürnberg)
--
--
To unsubscribe from thi
[v6 PATCH 10/21] x86/insn-eval: Do not use R/EBP as base if mod in ModRM is zero
Section 2.2.1.3 of the Intel 64 and IA-32 Architectures Software
Developer's Manual volume 2A states that when the mod part of the ModRM
byte is zero and R/EBP is specified in the R/M part of such bit, the value
of the aforementioned register should not be used in the address
computation. Instead, a 32-bit displacement is expected. The instruction
decoder takes care of setting the displacement to the expected value.
Returning -EDOM signals callers that they should ignore the value of such
register when computing the address encoded in the instruction operands.
Also, callers should exercise care to correctly interpret this particular
case. In IA-32e 64-bit mode, the address is given by the displacement plus
the value of the RIP. In IA-32e compatibility mode, the value of EIP is
ignored. This correction is done for our insn_get_addr_ref.
Cc: Dave Hansen
Cc: Adam Buchbinder
Cc: Colin Ian King
Cc: Lorenzo Stoakes
Cc: Qiaowei Ren
Cc: Arnaldo Carvalho de Melo
Cc: Masami Hiramatsu
Cc: Adrian Hunter
Cc: Kees Cook
Cc: Thomas Garnier
Cc: Peter Zijlstra
Cc: Borislav Petkov
Cc: Dmitry Vyukov
Cc: Ravi V. Shankar
Cc: x...@kernel.org
Signed-off-by: Ricardo Neri
---
arch/x86/lib/insn-eval.c | 25 +++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/arch/x86/lib/insn-eval.c b/arch/x86/lib/insn-eval.c
index cda6c71..ea10b03 100644
--- a/arch/x86/lib/insn-eval.c
+++ b/arch/x86/lib/insn-eval.c
@@ -250,6 +250,14 @@ static int get_reg_offset(struct insn *insn, struct
pt_regs *regs,
switch (type) {
case REG_TYPE_RM:
regno = X86_MODRM_RM(insn->modrm.value);
+ /* if mod=0, register R/EBP is not used in the address
+* computation. Instead, a 32-bit displacement is expected;
+* the instruction decoder takes care of reading such
+* displacement. This is true for both R/EBP and R13, as the
+* REX.B bit is not decoded.
+*/
+ if (regno == 5 && X86_MODRM_MOD(insn->modrm.value) == 0)
+ return -EDOM;
if (X86_REX_B(insn->rex_prefix.value))
regno += 8;
break;
@@ -599,9 +607,22 @@ void __user *insn_get_addr_ref(struct insn *insn, struct
pt_regs *regs)
eff_addr = base + indx * (1 << X86_SIB_SCALE(sib));
} else {
addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
- if (addr_offset < 0)
+ /* -EDOM means that we must ignore the address_offset.
+* The only case in which we see this value is when
+* R/M points to R/EBP. In such a case, in 64-bit mode
+* the effective address is relative to tho RIP.
+*/
+ if (addr_offset == -EDOM) {
+ eff_addr = 0;
+#ifdef CONFIG_X86_64
+ if (user_64bit_mode(regs))
+ eff_addr = (long)regs->ip;
+#endif
+ } else if (addr_offset < 0) {
goto out_err;
- eff_addr = regs_get_register(regs, addr_offset);
+ } else {
+ eff_addr = regs_get_register(regs, addr_offset);
+ }
}
eff_addr += insn->displacement.value;
}
--
2.9.3
--
To unsubscribe from this list: send the line "unsubscribe linux-msdos" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
