Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils

[Ricardo Neri]

On Sun, 2016-12-25 at 15:17 +0900, Masami Hiramatsu wrote:
> Hi Ricado,
>
> On Fri, 23 Dec 2016 17:37:41 -0800
> Ricardo Neri  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-utils.c file. The reason
> > to not relocate these utilities for insn.c is that the latter solely
> > analyses instructions given by a struct insn. The file insn-utils.c intends
> > to be used when, for instance, determining addresses from the contents
> > of the general purpose registers.
> >
> > To avoid creating a new insn-utils.h, insn.h is used. One caveat, however,
> > is that several #include's were needed by the utility functions.
> >
> > Functions are simply relocated. There are not functional or indentation
> > changes.
>
> Thank you for your great work! :)

Thanks a lot for taking the time to review!
> > arch/x86/include/asm/insn.h |   6 ++
> >  arch/x86/lib/Makefile       |   2 +-
> >  arch/x86/lib/insn-utils.c   | 148 ++++++++++++++++++++++++++++++++++++++++++++
> >  arch/x86/mm/mpx.c           | 136 +---------------------------------------
> >  4 files changed, 156 insertions(+), 136 deletions(-)
> >  create mode 100644 arch/x86/lib/insn-utils.c
> >
> > diff --git a/arch/x86/include/asm/insn.h b/arch/x86/include/asm/insn.h
> > index b3e32b0..9dc9d42 100644
> > --- a/arch/x86/include/asm/insn.h
> > +++ b/arch/x86/include/asm/insn.h
> > @@ -22,6 +22,10 @@
> >
> > /* insn_attr_t is defined in inat.h */
> >  #include <asm/inat.h>
> > +#include <linux/compiler.h>
> > +#include <linux/bug.h>
> > +#include <linux/err.h>
> > +#include <asm/ptrace.h>
> >
> > struct insn_field {
> >    union {
> > @@ -106,6 +110,8 @@ extern void insn_get_sib(struct insn *insn);
> >  extern void insn_get_displacement(struct insn *insn);
> >  extern void insn_get_immediate(struct insn *insn);
> >  extern void insn_get_length(struct insn *insn);
> > +extern void __user *insn_get_addr_ref(struct insn *insn, struct pt_regs *regs);
> > +extern int get_reg_offset_rm(struct insn *insn, struct pt_regs *regs);
>
> Could you also rename this to add "insn_" prefix?

Sure. This should have the prefix. I missed this.
> Other part looks good to me :)
> (btw, I saw a kbuild bot warning, would you also test it with
>  CONFIG_X86_DECODER_SELFTEST=y?)

I will do.

Thanks and BR,
Ricardo
> Thanks again!
> > /* Attribute will be determined after getting ModRM (for opcode groups) */
> >  static inline void insn_get_attribute(struct insn *insn)
> > diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
> > index 34a7413..0d01d82 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-utils.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-utils.c b/arch/x86/lib/insn-utils.c
> > new file mode 100644
> > index 0000000..598bbd6
> > --- /dev/null
> > +++ b/arch/x86/lib/insn-utils.c
> > @@ -0,0 +1,148 @@
> > +/*
> > -
> > - Utility functions for x86 operand and address decoding
> > -
> > -
> > -
> > - Copyright (C) Intel Corporation 2016
> > - */
> >
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <asm/inat.h>
> > +#include <asm/insn.h>
> > -
> >
> > +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,
> > -
> > 	  enum reg_type type)
> >
> > +{
> > - int regno = 0;
> > -
> > - static const int regoff[] = {
> > -
> > offsetof(struct pt_regs, ax),
> > -
> > offsetof(struct pt_regs, cx),
> > -
> > offsetof(struct 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];
> >
> > +}
> > -
> >
> > +int 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
> > -
> > - for rm!=3 calculates the address using SIB and Disp
> > - */
> >
> > +void __user *insn_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;
> >
> > +}
> > diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c
> > index 71681d0..32ba342 100644
> > --- a/arch/x86/mm/mpx.c
> > +++ b/arch/x86/mm/mpx.c
> > @@ -59,140 +59,6 @@ static unsigned long mpx_mmap(unsigned long len)
> >    return addr;
> >  }
> >
> > -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,
> > -
> > 	  enum reg_type type)
> >
> > -{
> > - int regno = 0;
> > -
> > - static const int regoff[] = {
> > -
> > offsetof(struct pt_regs, ax),
> > -
> > offsetof(struct pt_regs, cx),
> > -
> > offsetof(struct 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(&insn, regs);
> > - info->si_addr = insn_get_addr_ref(&insn, regs);
> > /*
> > - We were not able to extract an address from the instruction,
> > - probably because there was something invalid in it.
> >
> > -- 
> > 2.9.3