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:	Wed, 9 Apr 2014 18:42:04 +0200
From:	Oleg Nesterov <oleg@...hat.com>
To:	Jim Keniston <jkenisto@...ux.vnet.ibm.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 04/08, Jim Keniston wrote:
>
> On Mon, 2014-04-07 at 16:27 +0200, Oleg Nesterov wrote:
> >
> > +#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.

No-no-no, please do not ask me to remove it ;) I understand that this
is subjective, but to me it helps. Please see below.

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

Heh. I blindly copied the opcodes from the manual, I didn't even realize
that the range is continuous.

But gcc is more clever than me, I removed "static" and

	is_cond_jmp_opcode:
		pushq   %rbp    #
		movq    %rsp, %rbp      #,
		call    mcount
		leave
		subl    $112, %edi      #, tmp62
		cmpb    $15, %dil       #, tmp62
		setbe   %al     #, tmp64
		ret

shows that at least this doesn't make the code worse.


> I would keep the XF macro (although the !! operation to convert non-zero
> to 1 isn't strictly needed)

Well, "!!" is commonly used to make clear the fact we need a boolean,
even if this is not strictly needed.

Besides, in this case it is needed for "!=" above, or we would need to do

	- XF(SF) != XF(OF)
	+ !!XF(SF) != !!XF(OF)

> and just do an explicit 16-case switch for
> check_jmp_cond().

I'd prefer to keep these macros. They are only used by is_cond_jmp_opcode()
and check_jmp_cond(), and I really think they make the code more readable.

And more editable. To remind, I am going to add the support for j*cxz/loop
later. Just for completeness, we do not need this to fix the bug. In this
case I will simply add

	case 0xe3: DO(check_rcx(auprobe, regs))

at the end of CASE_COND and that is all.

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

OK. I changed this line

	unsigned long disp = (long)auprobe->ttt.disp;

to make it clear that yes, disp can be negative. Or we could simply use s32,
but then

	regs->ip = new_ip + disp;

could look equally confusing.

Thanks,

Oleg.

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