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

[Masami Hiramatsu <mhiramat@kernel.org>]

Hi Ricado,

On Fri, 23 Dec 2016 17:37:41 -0800
Ricardo Neri <ricardo.neri-calderon@linux.intel.com> 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 <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?

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


-- 
Masami Hiramatsu <mhiramat@kernel.org>