linux-kernel - Re: [RFC PATCH 1/6] uprobes/x86: Emulate unconditional rip-relative jmp's

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 08 Apr 2014 13:36:51 -0700
From:	Jim Keniston <jkenisto@...ux.vnet.ibm.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Ingo Molnar <mingo@...e.hu>,
	Srikar Dronamraju <srikar@...ux.vnet.ibm.com>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Anton Arapov <aarapov@...hat.com>,
	David Long <dave.long@...aro.org>,
	Denys Vlasenko <dvlasenk@...hat.com>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Jonathan Lebon <jlebon@...hat.com>,
	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/6] uprobes/x86: Emulate unconditional
 rip-relative jmp's

On Sun, 2014-04-06 at 22:16 +0200, Oleg Nesterov wrote:
> 0xeb and 0xe9. Anything else?

For unconditional rip-relative jumps, yes, I'm pretty sure that's it.

> 
> TODO: move ->fixup into the union along with rip_rela_target_address.
> 
> Reported-by: Jonathan Lebon <jlebon@...hat.com>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  arch/x86/include/asm/uprobes.h |    8 +++++++-
>  arch/x86/kernel/uprobes.c      |   40 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> index 9f8210b..cca62c5 100644
> --- a/arch/x86/include/asm/uprobes.h
> +++ b/arch/x86/include/asm/uprobes.h
> @@ -44,9 +44,15 @@ struct arch_uprobe {
>  	u16				fixups;
>  	const struct uprobe_xol_ops	*ops;
> 
> +	union {
>  #ifdef CONFIG_X86_64
> -	unsigned long			rip_rela_target_address;
> +		unsigned long			rip_rela_target_address;
>  #endif
> +		struct {
> +			s32	disp;
> +			u8	ilen;
> +		}				ttt;

Are you planning to stick with ttt as the name/prefix for all this
jump-emulation code?  Seems like you could come up with something more
descriptive without adding a lot of characters.
 
> +	};
>  };
> 
>  struct arch_uprobe_task {
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 1ea7e1a..32ab147 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -466,6 +466,42 @@ static struct uprobe_xol_ops default_xol_ops = {
>  	.post_xol = default_post_xol_op,
>  };
> 
> +static bool ttt_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	regs->ip += auprobe->ttt.ilen + auprobe->ttt.disp;
> +	return true;
> +}
> +
> +static struct uprobe_xol_ops ttt_xol_ops = {
> +	.emulate  = ttt_emulate_op,
> +};
> +
> +static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> +{
> +
> +	switch (OPCODE1(insn)) {
> +	case 0xeb:	/* jmp 8 */
> +	case 0xe9:	/* jmp 32 */
> +		break;
> +	default:
> +		return -ENOSYS;

-ENOSYS looks like an error return, as opposed to what it is, the normal
return when the probed instruction is something other than a jump.  This
gets more bewildering as you add patches and this switch grows and gets
more complicated.  Add a comment here?

> +	}
> +
> +	/* has the side-effect of processing the entire instruction */
> +	insn_get_length(insn);
> +	if (WARN_ON_ONCE(!insn_complete(insn)))
> +		return -ENOEXEC;
> +
> +	auprobe->ttt.ilen = insn->length;
> +	auprobe->ttt.disp = insn->moffset1.value;
> +	/* so far we assume that it fits into ->moffset1 */
> +	if (WARN_ON_ONCE(insn->moffset2.nbytes))
> +		return -ENOEXEC;

s/moffset1/immediate/ -- which you've already addressed.

> +
> +	auprobe->ops = &ttt_xol_ops;
> +	return 0;
> +}
> +
>  /**
>   * arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
>   * @mm: the probed address space.
> @@ -483,6 +519,10 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	if (ret)
>  		return ret;
> 
> +	ret = ttt_setup_xol_ops(auprobe, &insn);
> +	if (ret == 0 || ret != ENOSYS)

This looks wrong in a couple of ways:
a. I think you intend to compare against -ENOSYS, not ENOSYS.
b. Given the (ret != [-]ENOSYS) test, the  (ret == 0) test is
superfluous.


> +		return ret;
> +
>  	/*
>  	 * Figure out which fixups arch_uprobe_post_xol() will need to perform,
>  	 * and annotate arch_uprobe->fixups accordingly. To start with, ->fixups

Jim

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/