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

2017-01-03 Thread Ricardo Neri 
On Tue, 2017-01-03 at 08:44 -0800, Dave Hansen wrote:
> On 12/23/2016 05:37 PM, 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.
> 
> I haven't looked at this in detail, but as long as this is pretty much a
> straight code move, I don't see any issues with it from an MPX
> perspective.  I'm glad to see it getting reused.

Yes, this is only a relocation of code.
> 
> Feel free to add my Acked-by on it if you like.

Great! Thanks!
Ricardo


--
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

2017-01-03 Thread Dave Hansen 
On 12/23/2016 05:37 PM, 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.

I haven't looked at this in detail, but as long as this is pretty much a
straight code move, I don't see any issues with it from an MPX
perspective.  I'm glad to see it getting reused.

Feel free to add my Acked-by on it if you like.
--
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

2016-12-27 Thread 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 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> >  
> >  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 000..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 
> > +#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,
> > + 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

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

2016-12-24 Thread Masami Hiramatsu 
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! :)

> ---
>  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 
> +#include 
> +#include 
> +#include 
> +#include 
>  
>  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?

Other part looks good to me :)
(btw, I saw a kbuild bot warning, would you also test it with
 CONFIG_X86_DECODER_SELFTEST=y?)

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 000..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 
> +#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,
> +   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