Re: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM
On Wed, 25 Jan 2017 22:07:16 -0800
Ricardo Neri <ricardo.neri-calde...@linux.intel.com> wrote:
> Hi Masami,
>
> On Thu, 2017-01-26 at 11:11 +0900, Masami Hiramatsu wrote:
> > On Wed, 25 Jan 2017 12:23:47 -0800
> > Ricardo Neri <ricardo.neri-calde...@linux.intel.com> wrote:
> >
> > > The function insn_get_reg_offset requires a type to indicate whether
> > > the returned offset is that given by by the ModRM or the SIB byte.
> > > Callers of this function would need the definition of the type struct.
> > > This is not needed. Instead, auxiliary functions can be defined for
> > > this purpose.
> > >
> > > When the operand is a register, the emulation code for User-Mode
> > > Instruction Prevention needs to know the offset of the register indicated
> > > in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> > > function for this purpose.
> >
> > Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?
>
> Do you mean exporting the structure that I mention above? The problem
> that I am trying to solve is that callers sometimes want to know the
> offset of the register encoded in the SiB or the ModRM bytes. I could
> use something
>
> insn_get_reg_offset(insn, regs, INSN_TYPE_MODRM)
> insn_get_reg_offset(insn, regs, INSN_TYPE_SIB)
>
> Instead, I opted for
>
> insn_get_reg_offset_rm(insn, regs)
> insn_get_reg_offset_sib(insn, regs)
>
> to avoid exposing an enum with the INSN_TYPE_MODRM, INSN_TYPE_SIB.
OK, if so, I think you should export both of them at once, not only
insn_get_reg_offset_rm().
Thank you,
>
> If you feel that the former makes more sense, I can change the
> implementation.
>
> Thanks and BR,
> Ricardo
> >
> > Thank you,
> >
> > >
> > > Cc: Dave Hansen <dave.han...@linux.intel.com>
> > > Cc: Adam Buchbinder <adam.buchbin...@gmail.com>
> > > Cc: Colin Ian King <colin.k...@canonical.com>
> > > Cc: Lorenzo Stoakes <lstoa...@gmail.com>
> > > Cc: Qiaowei Ren <qiaowei@intel.com>
> > > Cc: Arnaldo Carvalho de Melo <a...@redhat.com>
> > > Cc: Masami Hiramatsu <mhira...@kernel.org>
> > > Cc: Adrian Hunter <adrian.hun...@intel.com>
> > > Cc: Kees Cook <keesc...@chromium.org>
> > > Cc: Thomas Garnier <thgar...@google.com>
> > > Cc: Peter Zijlstra <pet...@infradead.org>
> > > Cc: Borislav Petkov <b...@suse.de>
> > > Cc: Dmitry Vyukov <dvyu...@google.com>
> > > Cc: Ravi V. Shankar <ravi.v.shan...@intel.com>
> > > Cc: x...@kernel.org
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calde...@linux.intel.com>
> > > ---
> > > arch/x86/include/asm/insn-kernel.h | 1 +
> > > arch/x86/lib/insn-kernel.c | 5 +
> > > 2 files changed, 6 insertions(+)
> > >
> > > diff --git a/arch/x86/include/asm/insn-kernel.h
> > > b/arch/x86/include/asm/insn-kernel.h
> > > index aef416a..3f34649 100644
> > > --- a/arch/x86/include/asm/insn-kernel.h
> > > +++ b/arch/x86/include/asm/insn-kernel.h
> > > @@ -12,5 +12,6 @@
> > > #include
> > >
> > > void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
> > >
> > > #endif /* _ASM_X86_INSN_KERNEL_H */
> > > diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> > > index 8072abe..267cab4 100644
> > > --- a/arch/x86/lib/insn-kernel.c
> > > +++ b/arch/x86/lib/insn-kernel.c
> > > @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct
> > > pt_regs *regs,
> > > return regoff[regno];
> > > }
> > >
> > > +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> > > +{
> > > + return get_reg_offset(insn, regs, REG_TYPE_RM);
> > > +}
> > > +
> > > /*
> > > * return the address being referenced be instruction
> > > * for rm=3 returning the content of the rm reg
> > > --
> > > 2.9.3
> > >
> >
> >
>
>
--
Masami Hiramatsu <mhira...@kernel.org>
--
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: [v3 PATCH 03/10] x86/mpx, x86/insn: Relocate insn util functions to a new insn-kernel
On Wed, 25 Jan 2017 12:23:46 -0800
Ricardo Neri <ricardo.neri-calde...@linux.intel.com> wrote:
> Other kernel submodules can benefit from using the utility functions
> defined in mpx.c to obtain the addresses and values of operands contained
> in the general purpose registers. An instance of this is the emulation code
> used for instructions protected by the Intel User-Mode Instruction
> Prevention feature.
>
> Thus, these functions are relocated to a new insn-kernel.c file. The reason
> to not relocate these utilities into insn.c is that the latter solely
> analyses instructions given by a struct insn without any knowledge of the
> kernel context. This new utilities insn-kernel.c aims to be used within the
> context of the kernel. For instance, it can be used to determine memory
> addresses as encoded in the contents of the general purpose registers.
What would you mean the "kernel context" here? Extracting the register offset
or an address by decoding instruction seems not depending on where the
context (pt_regs) in kernel or user...
Of course, this is a kind of "evaluation" of instruction, so it might be
better to split it to other file, but I think insn-eval.c is better.
Thank you,
>
> These utilities come with a separate header. This is to avoid taking insn.c
> out of sync from the instructions decoders under tools/obj and tools/perf.
> This also avoids adding cumbersome #ifdef's for the #include'd files
> required to decode instructions in a kernel context.
>
> Functions are simply relocated. There are not functional or indentation
> changes.
>
> Cc: Dave Hansen <dave.han...@linux.intel.com>
> Cc: Adam Buchbinder <adam.buchbin...@gmail.com>
> Cc: Colin Ian King <colin.k...@canonical.com>
> Cc: Lorenzo Stoakes <lstoa...@gmail.com>
> Cc: Qiaowei Ren <qiaowei@intel.com>
> Cc: Arnaldo Carvalho de Melo <a...@redhat.com>
> Cc: Masami Hiramatsu <mhira...@kernel.org>
> Cc: Adrian Hunter <adrian.hun...@intel.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Thomas Garnier <thgar...@google.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Dmitry Vyukov <dvyu...@google.com>
> Cc: Ravi V. Shankar <ravi.v.shan...@intel.com>
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri <ricardo.neri-calde...@linux.intel.com>
> ---
> arch/x86/include/asm/insn-kernel.h | 16
> arch/x86/lib/Makefile | 2 +-
> arch/x86/lib/insn-kernel.c | 147
> +
> arch/x86/mm/mpx.c | 140 +--
> 4 files changed, 166 insertions(+), 139 deletions(-)
> create mode 100644 arch/x86/include/asm/insn-kernel.h
> create mode 100644 arch/x86/lib/insn-kernel.c
>
> diff --git a/arch/x86/include/asm/insn-kernel.h
> b/arch/x86/include/asm/insn-kernel.h
> new file mode 100644
> index 000..aef416a
> --- /dev/null
> +++ b/arch/x86/include/asm/insn-kernel.h
> @@ -0,0 +1,16 @@
> +#ifndef _ASM_X86_INSN_KERNEL_H
> +#define _ASM_X86_INSN_KERNEL_H
> +/*
> + * A collection of utility functions for x86 instruction analysis to be
> + * used in a kernel context. Useful when, for instance, making sense
> + * of the registers indicated by operands.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +
> +void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> +
> +#endif /* _ASM_X86_INSN_KERNEL_H */
> diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> index 34a7413..d33eff1 100644
> --- a/arch/x86/lib/Makefile
> +++ b/arch/x86/lib/Makefile
> @@ -23,7 +23,7 @@ lib-y := delay.o misc.o cmdline.o cpu.o
> lib-y += usercopy_$(BITS).o usercopy.o getuser.o putuser.o
> lib-y += memcpy_$(BITS).o
> lib-$(CONFIG_RWSEM_XCHGADD_ALGORITHM) += rwsem.o
> -lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o
> +lib-$(CONFIG_INSTRUCTION_DECODER) += insn.o inat.o insn-kernel.o
> lib-$(CONFIG_RANDOMIZE_BASE) += kaslr.o
>
> obj-y += msr.o msr-reg.o msr-reg-export.o hweight.o
> diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> new file mode 100644
> index 000..8072abe
> --- /dev/null
> +++ b/arch/x86/lib/insn-kernel.c
> @@ -0,0 +1,147 @@
> +/*
> + * Utility functions for x86 operand and address decoding
> + *
> + * Copyright (C) Intel Corporation 2016
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +enum reg_type {
> + REG_TYPE_RM = 0,
> + REG_TYPE_INDEX,
> + REG_TYPE_BASE,
> +};
> +
> +static int get_reg_offset(struct insn *insn, struct pt_regs *regs
Re: [v3 PATCH 04/10] x86/insn-kernel: Add a function to obtain register offset in ModRM
On Wed, 25 Jan 2017 12:23:47 -0800
Ricardo Neri <ricardo.neri-calde...@linux.intel.com> wrote:
> The function insn_get_reg_offset requires a type to indicate whether
> the returned offset is that given by by the ModRM or the SIB byte.
> Callers of this function would need the definition of the type struct.
> This is not needed. Instead, auxiliary functions can be defined for
> this purpose.
>
> When the operand is a register, the emulation code for User-Mode
> Instruction Prevention needs to know the offset of the register indicated
> in the r/m part of the ModRM byte. Thus, start by adding an auxiliary
> function for this purpose.
Hmm, why wouldn't you just rename it to insn_get_reg_offset() and export it?
Thank you,
>
> Cc: Dave Hansen <dave.han...@linux.intel.com>
> Cc: Adam Buchbinder <adam.buchbin...@gmail.com>
> Cc: Colin Ian King <colin.k...@canonical.com>
> Cc: Lorenzo Stoakes <lstoa...@gmail.com>
> Cc: Qiaowei Ren <qiaowei@intel.com>
> Cc: Arnaldo Carvalho de Melo <a...@redhat.com>
> Cc: Masami Hiramatsu <mhira...@kernel.org>
> Cc: Adrian Hunter <adrian.hun...@intel.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Thomas Garnier <thgar...@google.com>
> Cc: Peter Zijlstra <pet...@infradead.org>
> Cc: Borislav Petkov <b...@suse.de>
> Cc: Dmitry Vyukov <dvyu...@google.com>
> Cc: Ravi V. Shankar <ravi.v.shan...@intel.com>
> Cc: x...@kernel.org
> Signed-off-by: Ricardo Neri <ricardo.neri-calde...@linux.intel.com>
> ---
> arch/x86/include/asm/insn-kernel.h | 1 +
> arch/x86/lib/insn-kernel.c | 5 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/arch/x86/include/asm/insn-kernel.h
> b/arch/x86/include/asm/insn-kernel.h
> index aef416a..3f34649 100644
> --- a/arch/x86/include/asm/insn-kernel.h
> +++ b/arch/x86/include/asm/insn-kernel.h
> @@ -12,5 +12,6 @@
> #include
>
> void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
>
> #endif /* _ASM_X86_INSN_KERNEL_H */
> diff --git a/arch/x86/lib/insn-kernel.c b/arch/x86/lib/insn-kernel.c
> index 8072abe..267cab4 100644
> --- a/arch/x86/lib/insn-kernel.c
> +++ b/arch/x86/lib/insn-kernel.c
> @@ -95,6 +95,11 @@ static int get_reg_offset(struct insn *insn, struct
> pt_regs *regs,
> return regoff[regno];
> }
>
> +int insn_get_reg_offset_rm(struct insn *insn, struct pt_regs *regs)
> +{
> + return get_reg_offset(insn, regs, REG_TYPE_RM);
> +}
> +
> /*
> * return the address being referenced be instruction
> * for rm=3 returning the content of the rm reg
> --
> 2.9.3
>
--
Masami Hiramatsu <mhira...@kernel.org>
--
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: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
t pt_regs, dx),
> - offsetof(struct pt_regs, bx),
> - offsetof(struct pt_regs, sp),
> - offsetof(struct pt_regs, bp),
> - offsetof(struct pt_regs, si),
> - offsetof(struct pt_regs, di),
> -#ifdef CONFIG_X86_64
> - offsetof(struct pt_regs, r8),
> - offsetof(struct pt_regs, r9),
> - offsetof(struct pt_regs, r10),
> - offsetof(struct pt_regs, r11),
> - offsetof(struct pt_regs, r12),
> - offsetof(struct pt_regs, r13),
> - offsetof(struct pt_regs, r14),
> - offsetof(struct pt_regs, r15),
> -#endif
> - };
> - int nr_registers = ARRAY_SIZE(regoff);
> - /*
> - * Don't possibly decode a 32-bit instructions as
> - * reading a 64-bit-only register.
> - */
> - if (IS_ENABLED(CONFIG_X86_64) && !insn->x86_64)
> - nr_registers -= 8;
> -
> - switch (type) {
> - case REG_TYPE_RM:
> - regno = X86_MODRM_RM(insn->modrm.value);
> - if (X86_REX_B(insn->rex_prefix.value))
> - regno += 8;
> - break;
> -
> - case REG_TYPE_INDEX:
> - regno = X86_SIB_INDEX(insn->sib.value);
> - if (X86_REX_X(insn->rex_prefix.value))
> - regno += 8;
> - /*
> - * If mod !=3, SP is not used as index. Check is done after
> - * looking at REX.X This is because R12 can be used as an
> - * index.
> - */
> - if (regno == 4 && X86_MODRM_RM(insn->modrm.value) != 3)
> - return 0;
> - break;
> -
> - case REG_TYPE_BASE:
> - regno = X86_SIB_BASE(insn->sib.value);
> - if (regno == 5 && X86_MODRM_RM(insn->modrm.value) == 0) {
> - WARN_ONCE(1, "An explicit displacement is required when
> %sBP used as SIB base.",
> - (IS_ENABLED(CONFIG_X86_64) && insn->x86_64) ?
> - "R13 or R" : "E");
> - return -EINVAL;
> - }
> -
> - if (X86_REX_B(insn->rex_prefix.value))
> - regno += 8;
> - break;
> -
> - default:
> - pr_err("invalid register type");
> - BUG();
> - break;
> - }
> -
> - if (regno >= nr_registers) {
> - WARN_ONCE(1, "decoded an instruction with an invalid register");
> - return -EINVAL;
> - }
> - return regoff[regno];
> -}
> -
> -/*
> - * return the address being referenced be instruction
> - * for rm=3 returning the content of the rm reg
> - * for rm!=3 calculates the address using SIB and Disp
> - */
> -static void __user *mpx_get_addr_ref(struct insn *insn, struct pt_regs *regs)
> -{
> - unsigned long addr, base, indx;
> - int addr_offset, base_offset, indx_offset;
> - insn_byte_t sib;
> -
> - insn_get_modrm(insn);
> - insn_get_sib(insn);
> - sib = insn->sib.value;
> -
> - if (X86_MODRM_MOD(insn->modrm.value) == 3) {
> - addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> - if (addr_offset < 0)
> - goto out_err;
> - addr = regs_get_register(regs, addr_offset);
> - } else {
> - if (insn->sib.nbytes) {
> - base_offset = get_reg_offset(insn, regs, REG_TYPE_BASE);
> - if (base_offset < 0)
> - goto out_err;
> -
> - indx_offset = get_reg_offset(insn, regs,
> REG_TYPE_INDEX);
> - if (indx_offset < 0)
> - goto out_err;
> -
> - base = regs_get_register(regs, base_offset);
> - if (indx_offset)
> - indx = regs_get_register(regs, indx_offset);
> - else
> - indx = 0;
> - addr = base + indx * (1 << X86_SIB_SCALE(sib));
> - } else {
> - addr_offset = get_reg_offset(insn, regs, REG_TYPE_RM);
> - if (addr_offset < 0)
> - goto out_err;
> - addr = regs_get_register(regs, addr_offset);
> - }
> - addr += insn->displacement.value;
> - }
> - return (void __user *)addr;
> -out_err:
> - return (void __user *)-1;
> -}
> -
> static int mpx_insn_decode(struct insn *insn,
> struct pt_regs *regs)
> {
> @@ -305,7 +171,7 @@ siginfo_t *mpx_generate_siginfo(struct pt_regs *regs)
> info->si_signo = SIGSEGV;
> info->si_errno = 0;
> info->si_code = SEGV_BNDERR;
> - info->si_addr = mpx_get_addr_ref(, regs);
> + info->si_addr = insn_get_addr_ref(, regs);
> /*
>* We were not able to extract an address from the instruction,
>* probably because there was something invalid in it.
> --
> 2.9.3
>
--
Masami Hiramatsu <mhira...@kernel.org>
--
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
