Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932898AbcL0WgP (ORCPT ); Tue, 27 Dec 2016 17:36:15 -0500 Received: from mga07.intel.com ([134.134.136.100]:11938 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753801AbcL0WgG (ORCPT ); Tue, 27 Dec 2016 17:36:06 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,418,1477983600"; d="scan'208";a="802790782" Message-ID: <1482878165.106950.12.camel@ranerica-desktop> Subject: Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils From: Ricardo Neri To: Masami Hiramatsu Cc: Ingo Molnar , Thomas Gleixner , Borislav Petkov , Andy Lutomirski , Peter Zijlstra , linux-kernel@vger.kernel.org, x86@kernel.org, linux-msdos@vger.kernel.org, wine-devel@winehq.org, Dave Hansen , Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov , "Ravi V . Shankar" Date: Tue, 27 Dec 2016 14:36:05 -0800 In-Reply-To: <20161225151750.15651d28aabb2bba4dc10272@kernel.org> References: <20161224013745.108716-1-ricardo.neri-calderon@linux.intel.com> <20161224013745.108716-4-ricardo.neri-calderon@linux.intel.com> <20161225151750.15651d28aabb2bba4dc10272@kernel.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.10.4-0ubuntu2 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 12861 Lines: 403 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 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 > > +#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_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 > > > >