linux-kernel - Re: [PATCH v2 0/9] uprobes/x86: preparations to fix the reprel jmp/call handling.

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:	Fri, 04 Apr 2014 16:44:25 -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: [PATCH v2 0/9] uprobes/x86: preparations to fix the reprel
 jmp/call handling.

On Fri, 2014-04-04 at 21:32 +0200, Oleg Nesterov wrote:
> On 04/04, Oleg Nesterov wrote:
> >
> > Now let me send the draft RFC patch which fixes the "call" handling...
> 
> Damn. apparently I can't understand lib/insn.c...
> 
> Please see the draft below. Lets ignore 32bit tasks, lets ignore jmp's,
> please ignore how the (pseudo) code written, I'll change it anyway.
> 
> Questions:

So far, I have answers for just #1 and #2.

> 
> 	1. Why insn_get_displacement() doesn't work? See "HELP!!!"
> 	   below.

insn->moffset1.value seems to be what you want.

> 
> 	2. Do I use lib/insn.c correctly (ignoring displacement) ?

insn_get_length() has the side-effect of processing the entire
instruction, so just calling that should be sufficient.  Looks OK
otherwise -- but I checked very quickly.

More in a day or two.

Jim

> 
>            In particular, is 'turn this insn into "1: call 1b;"'
>            below correct?
> 
> 	3. Jim, do you still think it would be better to rewrite the
> 	   call insns using a scratch register ?
> 
> 	4. Is there other call insns with OPCODE1() != 0xe8 which
> 	   should be fixed too?
> 
> Thanks,
> 
> Oleg.
> 
> 
> 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;
> +	};
>  };
> 
>  struct arch_uprobe_task {
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index b820668..423ae86 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -461,6 +461,52 @@ 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)
> +{
> +
> +	if (put_user(regs->ip + auprobe->ttt.ilen, (long __user *)(regs->sp - 8)))
> +		return false;
> +
> +	regs->sp -= 8;
> +	regs->ip += auprobe->ttt.ilen + auprobe->ttt.disp;
> +
> +	return true;
> +}
> +
> +static int ttt_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	regs->sp += 8;
> +	if (ttt_emulate_op(auprobe, regs))
> +		return 0;
> +	return -ERESTART;
> +}
> +
> +static struct uprobe_xol_ops ttt_xol_ops = {
> +	.emulate  = ttt_emulate_op,
> +	.post_xol = ttt_post_xol_op,
> +};
> +
> +static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
> +{
> +	s32 *disp;
> +
> +	insn_get_length(insn);
> +	auprobe->ttt.ilen = insn->length;
> +
> +	insn_get_displacement(insn);
> +	auprobe->ttt.disp = insn->displacement.value;
> +	// HELP!!! the above doesn't work, ->displacement.value == 0
> +	auprobe->ttt.disp = *(s32 *)(auprobe->insn + 1);
> +
> +	// turn this insn into "1: call 1b;", we only need to xol it
> +	// to expand the stack if ->emulate() fails.
> +	disp = (void *)auprobe->insn + insn_offset_displacement(insn);
> +	*disp = -(s32)auprobe->ttt.ilen;
> +
> +	auprobe->ops = &ttt_xol_ops;
> +	return 0;
> +}
> +
>  /**
>   * arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
>   * @mm: the probed address space.
> @@ -484,6 +530,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	 * is either zero or it reflects rip-related fixups.
>  	 */
>  	switch (OPCODE1(&insn)) {
> +	case 0xe8:		/* call relative - has its own xol_ops */
> +		return ttt_setup_xol_ops(auprobe, &insn);
> +
>  	case 0x9d:		/* popf */
>  		auprobe->fixups |= UPROBE_FIX_SETF;
>  		break;
> @@ -493,9 +542,6 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
>  	case 0xca:
>  		fix_ip = false;
>  		break;
> -	case 0xe8:		/* call relative - Fix return addr */
> -		fix_call = true;
> -		break;
>  	case 0x9a:		/* call absolute - Fix return addr, not ip */
>  		fix_call = true;
>  		fix_ip = false;
> 


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