Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
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
Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
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.
Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
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 as > > +* reading a 64-bit-only regis
Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
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 REG_TYPE_INDEX: > + regno = X86_SIB_I
Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils
Hi Ricardo, [auto build test WARNING on tip/auto-latest] [also build test WARNING on v4.9 next-20161223] [cannot apply to tip/x86/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Ricardo-Neri/x86-enable-User-Mode-Instruction-Prevention/20161224-094338 config: x86_64-randconfig-x008-201651 (attached as .config) compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): >> warning: objtool: x86 instruction decoder differs from kernel --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip