linux-kernel - Re: [RFC PATCH v2 5/6] uprobes/x86: Emulate rip-relative conditional "short" 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 15:53:06 -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 v2 5/6] uprobes/x86: Emulate rip-relative
 conditional "short" jmp's

On Mon, 2014-04-07 at 16:27 +0200, Oleg Nesterov wrote:
> On 04/06, Oleg Nesterov wrote:
> >
> > Incomplete, lacks "jcxz". Simple to fix. Anything else?
> 
> Please see v2 below. Simplify the preprocessor hacks.
> 
> -------------------------------------------------------------------------------
> Subject: [RFC PATCH v2 5/6] uprobes/x86: Emulate rip-relative conditional "short" jmp's
> 
> Incomplete, lacks "jcxz". Simple to fix. Anything else?
> 
> Reported-by: Jonathan Lebon <jlebon@...hat.com>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
>  arch/x86/kernel/uprobes.c |   61 +++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 59 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 9283024..3865d8b 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -466,18 +466,72 @@ static bool ttt_is_call(struct arch_uprobe *auprobe)
>  	return auprobe->ttt.opc1 == 0xe8;
>  }
> 
> +#define CASE_COND					\
> +	COND(70, 71, XF(OF))				\
> +	COND(72, 73, XF(CF))				\
> +	COND(74, 75, XF(ZF))				\
> +	COND(78, 79, XF(SF))				\
> +	COND(7a, 7b, XF(PF))				\
> +	COND(76, 77, XF(CF) || XF(ZF))			\
> +	COND(7c, 7d, XF(SF) != XF(OF))			\
> +	COND(7e, 7f, XF(ZF) || XF(SF) != XF(OF))
> +
> +#define COND(op_y, op_n, expr)				\
> +	case 0x ## op_y: DO((expr) != 0)		\
> +	case 0x ## op_n: DO((expr) == 0)
> +
> +#define XF(xf)	(!!(flags & X86_EFLAGS_ ## xf))

All this macro magic seems way more clever than it is legible.

Given that you're mapping 0f 8x to 7x (patch #6), is_cond_jmp_opcode()
could just be
	return (0x70 <= opcode && opcode <= 0x7f);

I would keep the XF macro (although the !! operation to convert non-zero
to 1 isn't strictly needed) and just do an explicit 16-case switch for
check_jmp_cond().

> +
> +static bool is_cond_jmp_opcode(u8 opcode)
> +{
> +	switch (opcode) {
> +	#define DO(expr)	\
> +		return true;
> +	CASE_COND
> +	#undef	DO
> +
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool check_jmp_cond(struct arch_uprobe *auprobe, struct pt_regs *regs)
> +{
> +	unsigned long flags = regs->flags;
> +
> +	switch (auprobe->ttt.opc1) {
> +	case 0x00:	/* not a conditional jmp */
> +		return true;
> +
> +	#define DO(expr)	\
> +		return expr;
> +	CASE_COND
> +	#undef	DO
> +
> +	default:
> +		BUG();
> +	}
> +}
> +
> +#undef	XF
> +#undef	COND
> +#undef	CASE_COND
> +
>  static bool ttt_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
>  {
>  	unsigned long new_ip = regs->ip += auprobe->ttt.ilen;
> +	unsigned long disp = auprobe->ttt.disp;

Looks like a negative ttt.disp will get sign-extended like you want, but
still, making disp unsigned here doesn't seem quite right.

> 
>  	if (ttt_is_call(auprobe)) {
>  		unsigned long new_sp = regs->sp - sizeof_long();
>  		if (copy_to_user((void __user *)new_sp, &new_ip, sizeof_long()))
>  			return false;
>  		regs->sp = new_sp;
> +	} else if (!check_jmp_cond(auprobe, regs)) {
> +		disp = 0;
>  	}
> 
> -	regs->ip = new_ip + auprobe->ttt.disp;
> +	regs->ip = new_ip + disp;
>  	return true;
>  }
> 
> @@ -536,8 +590,11 @@ static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
>  		ttt_clear_displacement(auprobe, insn);
>  		auprobe->ttt.opc1 = opc1;
>  		break;
> +
>  	default:
> -		return -ENOSYS;
> +		if (!is_cond_jmp_opcode(opc1))
> +			return -ENOSYS;
> +		auprobe->ttt.opc1 = opc1;
>  	}
> 
>  	auprobe->ttt.ilen = insn->length;


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