Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752380AbcLYGSH (ORCPT ); Sun, 25 Dec 2016 01:18:07 -0500 Received: from mail.kernel.org ([198.145.29.136]:48030 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752164AbcLYGSD (ORCPT ); Sun, 25 Dec 2016 01:18:03 -0500 Date: Sun, 25 Dec 2016 15:17:50 +0900 From: Masami Hiramatsu To: Ricardo Neri Cc: Ingo Molnar , Thomas Gleixner , Borislav Petkov , Andy Lutomirski , Peter Zijlstra , linux-kernel@vger.kernel.org, x86@kernel.org, , , Dave Hansen , Adam Buchbinder , Colin Ian King , Lorenzo Stoakes , Qiaowei Ren , Arnaldo Carvalho de Melo , Masami Hiramatsu , Adrian Hunter , Kees Cook , Thomas Garnier , Dmitry Vyukov , "Ravi V . Shankar" Subject: Re: [v2 3/7] x86/mpx, x86/insn: Relocate insn util functions to a new insn-utils Message-Id: <20161225151750.15651d28aabb2bba4dc10272@kernel.org> In-Reply-To: <20161224013745.108716-4-ricardo.neri-calderon@linux.intel.com> References: <20161224013745.108716-1-ricardo.neri-calderon@linux.intel.com> <20161224013745.108716-4-ricardo.neri-calderon@linux.intel.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.31; x86_64-redhat-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11928 Lines: 394 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 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 > -- Masami Hiramatsu