uprobes/x86: preparations to fix the reprel jmp/call handling.

Introduce arch_uprobe->ops pointing to the "struct uprobe_xol_ops", move the current UPROBE_FIX_{RIP*,IP,CALL} code into the default set of methods and change arch_uprobe_pre/post_xol() accordingly.

This way we can add the new uprobe_xol_ops's to handle the insns which need the special processing (rip-relative jmp/call at least).

TODO: An error from adjust_ret_addr() shouldn't be silently ignored, we should teach arch_uprobe_post_xol() or handle_singlestep() paths to restart the probed insn in this case. And probably "adjust" can be simplified and turned into set_ret_addr(). It seems that we do not really need copy_from_user(), we can always calculate the value we need to write into *regs->sp.

Signed-off-by Oleg Nesterov <[email protected]>

Reviewed-by Jim Keniston <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    7 ++-
 arch/x86/kernel/uprobes.c      |  107 +++++++++++++++++++++++++--------------
 2 files changed, 74 insertions(+), 40 deletions(-)

arch/x86/include/asm/uprobes.h +6 -1

index 3087ea9..9f8210b 100644
@@ -33,12 +33,17 @@ typedef u8 uprobe_opcode_t;
 #define UPROBE_SWBP_INSN		0xcc
 #define UPROBE_SWBP_INSN_SIZE		   1
 
+struct uprobe_xol_ops;
+
 struct arch_uprobe {
-	u16				fixups;
 	union {
 		u8			insn[MAX_UINSN_BYTES];
 		u8			ixol[MAX_UINSN_BYTES];
 	};
+
+	u16				fixups;
+	const struct uprobe_xol_ops	*ops;
+
 #ifdef CONFIG_X86_64
 	unsigned long			rip_rela_target_address;
 #endif

arch/x86/kernel/uprobes.c +64 -39

index 3bb4198..13ad8a3 100644
@@ -402,6 +402,64 @@ static int validate_insn_bits(struct arch_uprobe *auprobe, struct mm_struct *mm,
 }
 #endif /* CONFIG_X86_64 */
 
+struct uprobe_xol_ops {
+	bool	(*emulate)(struct arch_uprobe *, struct pt_regs *);
+	int	(*pre_xol)(struct arch_uprobe *, struct pt_regs *);
+	int	(*post_xol)(struct arch_uprobe *, struct pt_regs *);
+};
+
+static int default_pre_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	pre_xol_rip_insn(auprobe, regs, &current->utask->autask);
+	return 0;
+}
+
+/*
+ * Adjust the return address pushed by a call insn executed out of line.
+ */
+static int adjust_ret_addr(unsigned long sp, long correction)
+{
+	int rasize, ncopied;
+	long ra = 0;
+
+	if (is_ia32_task())
+		rasize = 4;
+	else
+		rasize = 8;
+
+	ncopied = copy_from_user(&ra, (void __user *)sp, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	ra += correction;
+	ncopied = copy_to_user((void __user *)sp, &ra, rasize);
+	if (unlikely(ncopied))
+		return -EFAULT;
+
+	return 0;
+}
+
+static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	struct uprobe_task *utask = current->utask;
+	long correction = (long)(utask->vaddr - utask->xol_vaddr);
+	int ret = 0;
+
+	handle_riprel_post_xol(auprobe, regs, &correction);
+	if (auprobe->fixups & UPROBE_FIX_IP)
+		regs->ip += correction;
+
+	if (auprobe->fixups & UPROBE_FIX_CALL)
+		ret = adjust_ret_addr(regs->sp, correction);
+
+	return ret;
+}
+
+static struct uprobe_xol_ops default_xol_ops = {
+	.pre_xol  = default_pre_xol_op,
+	.post_xol = default_post_xol_op,
+};
+
 /**
  * arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.

@@ -464,6 +522,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 	if (fix_call)
 		auprobe->fixups |= UPROBE_FIX_CALL;
 
+	auprobe->ops = &default_xol_ops;
 	return 0;
 }
 

@@ -485,33 +544,8 @@ int arch_uprobe_pre_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	if (test_tsk_thread_flag(current, TIF_BLOCKSTEP))
 		set_task_blockstep(current, false);
 
-	pre_xol_rip_insn(auprobe, regs, &utask->autask);
-	return 0;
-}
-
-/*
- * This function is called by arch_uprobe_post_xol() to adjust the return
- * address pushed by a call instruction executed out of line.
- */
-static int adjust_ret_addr(unsigned long sp, long correction)
-{
-	int rasize, ncopied;
-	long ra = 0;
-
-	if (is_ia32_task())
-		rasize = 4;
-	else
-		rasize = 8;
-
-	ncopied = copy_from_user(&ra, (void __user *)sp, rasize);
-	if (unlikely(ncopied))
-		return -EFAULT;
-
-	ra += correction;
-	ncopied = copy_to_user((void __user *)sp, &ra, rasize);
-	if (unlikely(ncopied))
-		return -EFAULT;
-
+	if (auprobe->ops->pre_xol)
+		return auprobe->ops->pre_xol(auprobe, regs);
 	return 0;
 }
 

@@ -560,11 +594,8 @@ bool arch_uprobe_xol_was_trapped(struct task_struct *t)
 int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
 	struct uprobe_task *utask = current->utask;
-	long correction;
-	int result = 0;
 
 	WARN_ON_ONCE(current->thread.trap_nr != UPROBE_TRAP_NR);
-
 	current->thread.trap_nr = utask->autask.saved_trap_nr;
 	/*
 	 * arch_uprobe_pre_xol() doesn't save the state of TIF_BLOCKSTEP

@@ -576,15 +607,9 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	else if (!(auprobe->fixups & UPROBE_FIX_SETF))
 		regs->flags &= ~X86_EFLAGS_TF;
 
-	correction = (long)(utask->vaddr - utask->xol_vaddr);
-	handle_riprel_post_xol(auprobe, regs, &correction);
-	if (auprobe->fixups & UPROBE_FIX_IP)
-		regs->ip += correction;
-
-	if (auprobe->fixups & UPROBE_FIX_CALL)
-		result = adjust_ret_addr(regs->sp, correction);
-
-	return result;
+	if (auprobe->ops->post_xol)
+		return auprobe->ops->post_xol(auprobe, regs);
+	return 0;
 }
 
 /* callback routine for handling exceptions. */

Quoted 7 lines · Oleg Nesterov

TODO: An error from adjust_ret_addr() shouldn't be silently ignored, we should teach arch_uprobe_post_xol() or handle_singlestep() paths to restart the probed insn in this case. And probably "adjust" can be simplified and turned into set_ret_addr(). It seems that we do not really need copy_from_user(), we can always calculate the value we need to write into *regs->sp.

It seems that you fixed this in 8/9, we don't need the TODO list in the description.

arch/x86/kernel/uprobes.c static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) · line 673

Quoted 4 lines

arch/x86/kernel/uprobes.c +4 -0

+	if (auprobe->ops->emulate)
+		return auprobe->ops->emulate(auprobe, regs);
+
+	/* TODO: move this code into ->emulate() hook */

If you think this can move into the emulate(),

you should do in this
patch.

Since the following code runs by default, there should be no problem to do that.

Quoted 10 lines · Masami Hiramatsu

Quoted 7 lines · Oleg Nesterov

TODO: An error from adjust_ret_addr() shouldn't be silently ignored, we should teach arch_uprobe_post_xol() or handle_singlestep() paths to restart the probed insn in this case. And probably "adjust" can be simplified and turned into set_ret_addr(). It seems that we do not really need copy_from_user(), we can always calculate the value we need to write into *regs->sp.

Quoted 2 lines

It seems that you fixed this in 8/9, we don't need the TODO list in the description.

Well, OK, I'll update the changelog and remove the "error ... ignored" part. Although to be honest, I do not understand why do you think it is bad to document the other problems you found while you were writing the patch.

Quoted 10 lines · Masami Hiramatsu

Quoted 7 lines · Oleg Nesterov

TODO: An error from adjust_ret_addr() shouldn't be silently ignored, we should teach arch_uprobe_post_xol() or handle_singlestep() paths to restart the probed insn in this case. And probably "adjust" can be simplified and turned into set_ret_addr(). It seems that we do not really need copy_from_user(), we can always calculate the value we need to write into *regs->sp.

Quoted 2 lines

It seems that you fixed this in 8/9, we don't need the TODO list in the description.

Well, OK, I'll update the changelog and remove the "error ... ignored" part. Although to be honest, I do not understand why do you think it is bad to document the other problems you found while you were writing the patch.

Quoted 6 lines

arch/x86/kernel/uprobes.c static bool __skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs) · line 673

Quoted 4 lines

arch/x86/kernel/uprobes.c +4 -0

+	if (auprobe->ops->emulate)
+		return auprobe->ops->emulate(auprobe, regs);
+
+	/* TODO: move this code into ->emulate() hook */

If you think this can move into the emulate(),

Yes sure,

Quoted 2 lines

you should do in this
patch.

No, sorry, I strongly disagree, this should come as a separate change, and only after "Emulate jmp's".

Quoted 2 lines

Since the following code runs by default, there should be no problem to do that.

Hmm. Not sure I understand "by default".

If you meant that this should
go into default_emulate_op() (which we do not have), then I strongly disagree again.

It should not, if nothing else we need to record insn->length somewhere, this should go into ttt_emulate_op() we add later. And it simply looks much more natural to handle jmp's and nop's together.

Masami, this time I simply can't understand your objections, please clarify.

Thanks,

Oleg.

Reviewed-by Masami Hiramatsu <[email protected]>

Quoted 2 lines · Masami Hiramatsu

Reviewed-by Masami Hiramatsu <[email protected]>

Thanks!

Does this mean that I can add the same into 6-8 you didn't ack explicitely ?

SIGILL after the failed arch_uprobe_post_xol() should only be used as a last resort, we should try to restart the probed insn if possible.

Currently only adjust_ret_addr() can fail, and this can only happen if another thread unmapped our stack after we executed "call" out-of-line. Most probably the application if buggy, but even in this case it can have a handler for SIGSEGV/etc. And in theory it can be even correct and do something non-trivial with its memory.

Of course we can't restart unconditionally, so arch_uprobe_post_xol() does this only if ->post_xol() returns -ERESTART even if currently this is the only possible error.

Note: this is not "perfect", we do not want the extra handler_chain() after restart, but I think this is the best solution we can realistically do without too much uglifications.

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/uprobes.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

arch/x86/kernel/uprobes.c +11 -4

index e72903e..b820668 100644
@@ -443,16 +443,17 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 {
 	struct uprobe_task *utask = current->utask;
 	long correction = (long)(utask->vaddr - utask->xol_vaddr);
-	int ret = 0;
 
 	handle_riprel_post_xol(auprobe, regs, &correction);
 	if (auprobe->fixups & UPROBE_FIX_IP)
 		regs->ip += correction;
 
-	if (auprobe->fixups & UPROBE_FIX_CALL)
-		ret = adjust_ret_addr(regs->sp, correction);
+	if (auprobe->fixups & UPROBE_FIX_CALL) {
+		if (adjust_ret_addr(regs->sp, correction))
+			return -ERESTART;
+	}
 
-	return ret;
+	return 0;
 }
 
 static struct uprobe_xol_ops default_xol_ops = {

@@ -599,6 +600,12 @@ int arch_uprobe_post_xol(struct arch_uprobe *auprobe, struct pt_regs *regs)
 		int err = auprobe->ops->post_xol(auprobe, regs);
 		if (err) {
 			arch_uprobe_abort_xol(auprobe, regs);
+			/*
+			 * Restart the probed insn. ->post_xol() must ensure
+			 * this is really possible if it returns -ERESTART.
+			 */
+			if (err == -ERESTART)
+				return 0;
 			return err;
 		}
 	}

Quoted 7 lines · Oleg Nesterov

Currently only adjust_ret_addr() can fail, and this can only happen if another thread unmapped our stack after we executed "call" out-of-line. Most probably the application if buggy, but even in this case it can have a handler for SIGSEGV/etc. And in theory it can be even correct and do something non-trivial with its memory.

Quoted 3 lines

Of course we can't restart unconditionally, so arch_uprobe_post_xol() does this only if ->post_xol() returns -ERESTART even if currently this is the only possible error.

When re-executing the call instruction, I'd think the stack pointer would be wrong the second time around, unless you pop off the return address from the first try.

Now let me send the draft RFC patch which fixes the "call" handling...

Quoted 2 lines · Oleg Nesterov

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:

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

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

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.

arch/x86/include/asm/uprobes.h +7 -1

index 9f8210b..cca62c5 100644
@@ -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 {

arch/x86/kernel/uprobes.c +49 -3

index b820668..423ae86 100644
@@ -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;

Quoted 3 lines · Oleg Nesterov

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

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

0xeb and 0xe9. Anything else?

TODO: move ->fixup into the union along with rip_rela_target_address.

Reported-by Jonathan Lebon <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    8 +++++++-
 arch/x86/kernel/uprobes.c      |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 1 deletions(-)

arch/x86/include/asm/uprobes.h +0 -0

index 9f8210b..cca62c5 100644
@@ -44,9 +44,15 @@ struct arch_uprobe {
 	u16				fixups;
 	const struct uprobe_xol_ops	*ops;

arch/x86/include/asm/uprobes.h +1 -0

+	};
 };
 
 struct arch_uprobe_task {

arch/x86/kernel/uprobes.c +9 -0

index 1ea7e1a..32ab147 100644
@@ -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,
+};

arch/x86/kernel/uprobes.c +5 -0

+
+	auprobe->ops = &ttt_xol_ops;
+	return 0;
+}
+
 /**
  * arch_uprobe_analyze_insn - instruction analysis including validity and fixups.
  * @mm: the probed address space.

Quoted 1 line · Oleg Nesterov

0xeb and 0xe9. Anything else?

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

Quoted 6 lines

arch/x86/include/asm/uprobes.h +0 -0

@@ -44,9 +44,15 @@ struct arch_uprobe {
 	u16				fixups;
 	const struct uprobe_xol_ops	*ops;
 

arch/x86/include/asm/uprobes.h struct arch_uprobe { · line 54

Quoted 9 lines

arch/x86/include/asm/uprobes.h +6 -1

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

Quoted 6 lines

arch/x86/include/asm/uprobes.h +6 -0

+static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn)
+{
+
+	switch (OPCODE1(insn)) {
+	case 0xeb:	/* jmp 8 */
+	case 0xe9:	/* jmp 32 */

arch/x86/include/asm/uprobes.h

Quoted 3 lines

arch/x86/include/asm/uprobes.h +3 -0

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

Quoted 4 lines

arch/x86/include/asm/uprobes.h +4 -0

+	/* has the side-effect of processing the entire instruction */
+	insn_get_length(insn);
+	if (WARN_ON_ONCE(!insn_complete(insn)))
+		return -ENOEXEC;

arch/x86/include/asm/uprobes.h

Quoted 6 lines

arch/x86/include/asm/uprobes.h +6 -0

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

arch/x86/include/asm/uprobes.h int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm, · line 523

Quoted 6 lines

arch/x86/include/asm/uprobes.h +2 -0

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

1. Add the trivial sizeof_long() helper and change other callers of is_ia32_task() to use it.

Quoted 3 lines · Oleg Nesterov

1. Add the trivial sizeof_long() helper and change other callers of is_ia32_task() to use it.

...

This hunk #3 doesn't apply for me. I can't find in your patch sets where you added the lines being replaced (and they weren't there originally).

After I fixed up this hunk, this patch and the rest applied OK.

arch/x86/kernel/uprobes.c if (ret) pr_crit("EMULATE: %lx -> %lx\n", ip, regs->ip);

Quoted 13 lines

arch/x86/kernel/uprobes.c +1 -4

@@ -450,10 +448,7 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 
 	if (auprobe->fixups & UPROBE_FIX_CALL) {
 		if (adjust_ret_addr(regs->sp, correction)) {
-			if (is_ia32_task())
-				regs->sp += 4;
-			else
-				regs->sp += 8;
+			regs->sp += sizeof_long();
 			return -ERESTART;
 		}
 	}

@@ -738,23 +733,21 @@ if (ret) pr_crit("EMULATE: %lx -> %lx\n", ip, regs->ip);

...

This modified hunk worked for me.

arch/x86/kernel/uprobes.c +3 -1

@@ -450,7 +448,9 @@ static int default_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs
 
 	if (auprobe->fixups & UPROBE_FIX_CALL) {
-		if (adjust_ret_addr(regs->sp, correction))
+		if (adjust_ret_addr(regs->sp, correction)) {
+			regs->sp += sizeof_long();
 			return -ERESTART;
+		}
 	}
 
 	return 0;

Jim

0xe8. Anything else?

Emulating of rip-relative call is trivial, we only need to additionally push the ret-address. If this fails, we execute this instruction out of line and this should trigger the trap, the probed application should die or the same insn will be restarted if a signal handler expands the stack. We do not even need ->post_xol() for this case.

But there is a corner (and almost theoretical) case: another thread can expand the stack right before we execute this insn out of line. In this case it hit the same problem we are trying to solve. So we simply turn the probed insn into "call 1f; 1:" and add ->post_xol() which restores ->sp and restarts.

Many thanks to Jonathan who finally found the standalone reproducer,

Reported-by Jonathan Lebon <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/include/asm/uprobes.h |    1 +
 arch/x86/kernel/uprobes.c      |   69 ++++++++++++++++++++++++++++++++++------
 2 files changed, 60 insertions(+), 10 deletions(-)

arch/x86/include/asm/uprobes.h +1 -0

index cca62c5..9528117 100644
@@ -51,6 +51,7 @@ struct arch_uprobe {
 		struct {
 			s32	disp;
 			u8	ilen;
+			u8	opc1;
 		}				ttt;
 	};
 };

arch/x86/kernel/uprobes.c +15 -1

index 3bcc121..9283024 100644
@@ -461,33 +461,85 @@ static struct uprobe_xol_ops default_xol_ops = {
 	.post_xol = default_post_xol_op,
 };
 
+static bool ttt_is_call(struct arch_uprobe *auprobe)
+{
+	return auprobe->ttt.opc1 == 0xe8;
+}
+
 static bool ttt_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
 {
-	regs->ip += auprobe->ttt.ilen + auprobe->ttt.disp;
+	unsigned long new_ip = regs->ip += auprobe->ttt.ilen;
+
+	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;
+	}
+
+	regs->ip = new_ip + auprobe->ttt.disp;
 	return true;
 }

arch/x86/kernel/uprobes.c +9 -0

+}
+
 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)
 {
+	u8 opc1 = OPCODE1(insn);
+
+	/* has the side-effect of processing the entire instruction */
+	insn_get_length(insn);
+	if (WARN_ON_ONCE(!insn_complete(insn)))
+		return -ENOEXEC;
 

arch/x86/kernel/uprobes.c +1 -8

+		break;
 	default:
 		return -ENOSYS;
 	}
 
-	/* 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 */

@@ -534,9 +586,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;

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

The following couple of paragraphs should be included in the code, perhaps merged with some of the related comments already there. Without it, the code is pretty hard to follow.

Quoted 11 lines

Emulating of rip-relative call is trivial, we only need to additionally push the ret-address. If this fails, we execute this instruction out of line and this should trigger the trap, the probed application should die or the same insn will be restarted if a signal handler expands the stack. We do not even need ->post_xol() for this case.

But there is a corner (and almost theoretical) case: another thread can expand the stack right before we execute this insn out of line. In this case it hit the same problem we are trying to solve. So we simply turn the probed insn into "call 1f; 1:" and add ->post_xol() which restores ->sp and restarts.

Can you explain under what circumstances emulation of the call instruction would fail? Will the copy_to_user() call that writes the return address to the stack grow the stack if necessary?

Will
single-stepping the mangled call instruction expand the stack?

Quoted 5 lines

arch/x86/kernel/uprobes.c +5 -0

+static int ttt_post_xol_op(struct arch_uprobe *auprobe, struct pt_regs *regs)
+{
+	BUG_ON(!ttt_is_call(auprobe));
+	/*
+	 * We can only get here if ttt_emulate_op() failed to push the return

arch/x86/kernel/uprobes.c static struct uprobe_xol_ops default_xol_ops = { · line 492

Quoted 4 lines

arch/x86/kernel/uprobes.c +4 -0

+	 * address _and_ another thread expanded our stack before the (mangled)
+	 * "call" insn was executed out-of-line. Just restore ->sp and restart.
+	 * We could also restore ->ip and try to call ttt_emulate_op() again.
+	 */

As I understand it, this comment assumes that the single-stepped call instruction can't grow the stack on its own, and will instead die with a SEGV or some such.

As noted above, I don't know if that's correct. (If
the single-stepped call doesn't die and doesn't grow the stack -- I'm not sure how that would happen -- then it seems like we have an infinite loop.)

arch/x86/kernel/uprobes.c static struct uprobe_xol_ops default_xol_ops = { · line 501

Quoted 5 lines

arch/x86/kernel/uprobes.c +5 -0

+static void ttt_clear_displacement(struct arch_uprobe *auprobe, struct insn *insn)
+{
+	/*
+	 * Turn this insn into "call 1f; 1:", this is what we will execute
+	 * out-of-line if ->emulate() fails.

Add comment: In the unlikely event that this mangled instruction is successfully single-stepped, we'll reset regs->ip to the beginning of the instruction so the probepoint is hit again and the original instruction is emulated (successfully this time, we assume).

Quoted 9 lines

static int ttt_setup_xol_ops(struct arch_uprobe *auprobe, struct insn *insn) {

arch/x86/kernel/uprobes.c +6 -0

+	u8 opc1 = OPCODE1(insn);
+
+	/* has the side-effect of processing the entire instruction */
+	insn_get_length(insn);
+	if (WARN_ON_ONCE(!insn_complete(insn)))
+		return -ENOEXEC;

arch/x86/kernel/uprobes.c static struct uprobe_xol_ops default_xol_ops = { · line 537

Quoted 10 lines

arch/x86/kernel/uprobes.c +5 -1

-	switch (OPCODE1(insn)) {
+	switch (opc1) {
 	case 0xeb:	/* jmp 8 */
 	case 0xe9:	/* jmp 32 */
 	case 0x90:	/* prefix* + nop; same as jmp with .disp = 0 */
 		break;
+
+	case 0xe8:	/* call relative */
+		ttt_clear_displacement(auprobe, insn);
+		auprobe->ttt.opc1 = opc1;

You set ttt.opc1 for call, and later for conditional jumps, but not for nop and unconditional jumps. Might confuse a maintainer down the road?

Quoted 3 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

Quoted 2 lines

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 3 lines

The following couple of paragraphs should be included in the code, perhaps merged with some of the related comments already there. Without it, the code is pretty hard to follow.

OK, I'll try to improve the comments somehow...

Quoted 12 lines

Quoted 11 lines

Emulating of rip-relative call is trivial, we only need to additionally push the ret-address. If this fails, we execute this instruction out of line and this should trigger the trap, the probed application should die or the same insn will be restarted if a signal handler expands the stack. We do not even need ->post_xol() for this case.

But there is a corner (and almost theoretical) case: another thread can expand the stack right before we execute this insn out of line. In this case it hit the same problem we are trying to solve. So we simply turn the probed insn into "call 1f; 1:" and add ->post_xol() which restores ->sp and restarts.

Quoted 3 lines

Can you explain under what circumstances emulation of the call instruction would fail? Will the copy_to_user() call that writes the return address to the stack grow the stack if necessary?

If necessary and if possible.

Quoted 2 lines

Will
single-stepping the mangled call instruction expand the stack?

In the likely case it won't. If copy_to_user() failes, then "call" should fail too (again, ignoring the potential race with another thread which can populate the memory ->sp - 8 points to).

Quoted 4 lines · Oleg Nesterov

Quoted 3 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

Oleg, thanks for responding to (if not necessarily agreeing with) all my suggestions. I think your code is solid, but I think we were wrong about one thing. See below.

Quoted 28 lines

Quoted 3 lines

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 3 lines

The following couple of paragraphs should be included in the code, perhaps merged with some of the related comments already there. Without it, the code is pretty hard to follow.

OK, I'll try to improve the comments somehow...

Quoted 12 lines

Quoted 11 lines

Emulating of rip-relative call is trivial, we only need to additionally push the ret-address. If this fails, we execute this instruction out of line and this should trigger the trap, the probed application should die or the same insn will be restarted if a signal handler expands the stack. We do not even need ->post_xol() for this case.

But there is a corner (and almost theoretical) case: another thread can expand the stack right before we execute this insn out of line. In this case it hit the same problem we are trying to solve. So we simply turn the probed insn into "call 1f; 1:" and add ->post_xol() which restores ->sp and restarts.

Quoted 3 lines

Can you explain under what circumstances emulation of the call instruction would fail? Will the copy_to_user() call that writes the return address to the stack grow the stack if necessary?

Quoted 8 lines

If necessary and if possible.

Quoted 2 lines

Will
single-stepping the mangled call instruction expand the stack?

In the likely case it won't. If copy_to_user() failes, then "call" should fail too (again, ignoring the potential race with another thread which can populate the memory ->sp - 8 points to).

I've experimented with putting a uprobe on a call instruction (in a tight loop, with no ret) and a kprobe on expand_stack(), and my results seem to indicate that a single-stepped call instruction will successfully grow the stack vma where a call to copy_to_user() won't.

So your trick of single-stepping the mangled call after the failed emulation, and then retrying the emulation should come into play in more than just theoretical situations. We'd need it any time the probed call needs to grow the stack vma.

I can send you the test kprobe/uprobe modules I used, if you like.

Bottom line: no further code changes, but this would affect your comments.

Jim

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Quoted 1 line

Good.

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

Or rather, *I think* it should truncate it (i.e. zero-extend to full width), but conceivably some CPUs can be buggy wrt that:
they can decide to modify only lower 16 bits of IP,
or even they can decided to use signed <offset16> but apply it to full-width RIP.

AMD manuals are not clear on what exactly should happen.

I am sure no one sane uses this form of branch instructions in 32-bit and 64-bit code.

I don't think we should be trying to support it "correctly" (we can just let program crash with SIGILL or something), we only need to make sure we don't overlook its existence and thus are not tricked into touching or modifying unrelated data.

Imagine that 66 e8 nn nn bytes are exactly at the end of
a page, and we wrongly assume that offset is 32-bit, not 16-bit.

Quoted 10 lines · Denys Vlasenko

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 5 lines

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

Quoted 11 lines · Masami Hiramatsu

Quoted 10 lines · Denys Vlasenko

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 7 lines

Quoted 5 lines

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

That's good that we decode it correctly, but there is more to it.

Call insn pushes return address to stack.

This "mutant 16-bit call", what should it push? Full RIP?
Truncated 16-bit IP? If yes, by how much does it advance RSP? +2? +8?
Hmm. Does it affect RSP or only its 16-bit lower part?

It's a can of worms! :)

At least, if we can trust Intel SDM, it says that depends on the operand-size (insn->opnd_bytes) and stack segment descriptor. Please check the SDM vol.1 6.2.2 Stack Alignment and vol.2a, 3.2 Instructions (A-M), CALL--Call Procedure. But we'd better check it on x86-32.

Quoted 5 lines · Masami Hiramatsu

At least, if we can trust Intel SDM, it says that depends on the operand-size (insn->opnd_bytes) and stack segment descriptor. Please check the SDM vol.1 6.2.2 Stack Alignment and vol.2a, 3.2 Instructions (A-M), CALL--Call Procedure. But we'd better check it on x86-32.

I am past trusting CPU manuals on this one:

By now I verified on the real hardware that AMD and Intel CPUs handle this insn differently in 64-bit mode: Intel ignores 0x66 prefix. AMD treats this insn the same as in 32-bit mode: as 16-bit insn.

(Should I submit a patch adding comment about it
in x86-opcode-map.txt?)

So there is no universally "correct" way to emulate it.

We, theoretically, can decode it differently *depending on actual CPU(s) on the system*... do we really want to go *that* far? I guess not.

Quoted 10 lines · Denys Vlasenko

Quoted 5 lines · Masami Hiramatsu

At least, if we can trust Intel SDM, it says that depends on the operand-size (insn->opnd_bytes) and stack segment descriptor. Please check the SDM vol.1 6.2.2 Stack Alignment and vol.2a, 3.2 Instructions (A-M), CALL--Call Procedure. But we'd better check it on x86-32.

I am past trusting CPU manuals on this one:

Quoted 6 lines

By now I verified on the real hardware that AMD and Intel CPUs handle this insn differently in 64-bit mode: Intel ignores 0x66 prefix. AMD treats this insn the same as in 32-bit mode: as 16-bit insn.

(Should I submit a patch adding comment about it
in x86-opcode-map.txt?)

Yeah, feel free to do so :)

Quoted 10 lines · Denys Vlasenko

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 5 lines

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

Quoted 9 lines · Masami Hiramatsu

Quoted 5 lines · Denys Vlasenko

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

Does it?

	"callw 1f; 1:\n"
	"rep; nop\n"

objdump:

	66 e8 00 00             callw  485 <_init-0x3ffed3>
	f3 90                   pause

if we probe this "callw", we copy MAX_INSN_BYTES into auprobe->insn, and after insn_get_length() (insn_complete() == T)

	// this is correct
	OPCODE1() == e8

	// this all looks wrong
	insn->length == 6
	insn->immediate.value == -1863122944
	insn->immediate.nbytes == 4

so it seems that lib/insn.c treats the next "pause" insn as the high 16 bits of address.

Quoted 34 lines · Oleg Nesterov

Quoted 9 lines · Masami Hiramatsu

Quoted 5 lines · Denys Vlasenko

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

Does it?

	"callw 1f; 1:\n"
	"rep; nop\n"

objdump:

	66 e8 00 00             callw  485 <_init-0x3ffed3>
	f3 90                   pause

if we probe this "callw", we copy MAX_INSN_BYTES into auprobe->insn, and after insn_get_length() (insn_complete() == T)

	// this is correct
	OPCODE1() == e8

	// this all looks wrong
	insn->length == 6
	insn->immediate.value == -1863122944
	insn->immediate.nbytes == 4

Quoted 2 lines

so it seems that lib/insn.c treats the next "pause" insn as the high 16 bits of address.

Or perhaps lib/insn.c is fine but objdump is wrong? And everything should work correctly? Although in this case I do not understand what this "callw" actually does.

	int main(void)
	{
		asm (
			"nop\n"

			"callw 1f; 1:\n"
			".byte 0\n"
			".byte 0\n"
		);

		return 0;
	}

this runs just fine. With or without gdb. And gdb shows that ->ip is incremented by 6 after "callw".

	int main(void)
	{
		asm (
			"nop\n"

			"callw 1f; 1:\n"
			".byte 10\n"
			".byte 20\n"
		);

		return 0;
	}

objdump:

	000000000040047c <main>:
	  40047c:       55                      push   %rbp
	  40047d:       48 89 e5                mov    %rsp,%rbp
	  400480:       90                      nop
	  400481:       66 e8 00 00             callw  485 <_init-0x3ffed3>
	  400485:       0a 14 b8                or     (%rax,%rdi,4),%dl
	  400488:       00 00                   add    %al,(%rax)
	  40048a:       00 00                   add    %al,(%rax)
	  40048c:       c9                      leaveq
	  40048d:       c3                      retq

run:

	$ ./t
	Segmentation fault (core dumped)

	$ gdb ./t core.*
	...
	#0  0x00000000144a0487 in ?? ()

0x144a0487 - 0x400481 == 0x140a0006, this matches the additional 2 .bytes treated as offset.

Quoted 12 lines · Masami Hiramatsu

Quoted 10 lines · Denys Vlasenko

Quoted 8 lines · Oleg Nesterov

Quoted 5 lines · Jim Keniston

Quoted 1 line · Oleg Nesterov

0xe8. Anything else?

No, I think e8 is the only call instruction uprobes will see.

Good.

Quoted 7 lines

Quoted 5 lines

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

No problem, insn.c can handle that too. :)

Does it?

	"callw 1f; 1:\n"
	"rep; nop\n"

objdump:

	66 e8 00 00             callw  485 <_init-0x3ffed3>
	f3 90                   pause

if we probe this "callw", we copy MAX_INSN_BYTES into auprobe->insn, and after insn_get_length() (insn_complete() == T)

	// this is correct
	OPCODE1() == e8	

	// this all looks wrong
	insn->length == 6
	insn->immediate.value == -1863122944
	insn->immediate.nbytes == 4

so it seems that lib/insn.c treats the next "pause" insn as the high 16 bits of address.

Oleg.

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

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

Or rather, *I think* it should truncate it (i.e. zero-extend to full width), but conceivably some CPUs can be buggy wrt that:
they can decide to modify only lower 16 bits of IP,
or even they can decided to use signed <offset16> but apply it to full-width RIP.

AMD manuals are not clear on what exactly should happen.

I am sure no one sane uses this form of branch instructions in 32-bit and 64-bit code.

I don't think we should be trying to support it "correctly" (we can just let program crash with SIGILL or something), we only need to make sure we don't overlook its existence and thus are not tricked into touching or modifying unrelated data.

Imagine that 66 e8 nn nn bytes are exactly at the end of
a page,

and we wrongly assume that offset is 32-bit, not 16-bit. 66 e8 nn nn.

Quoted 18 lines · Denys Vlasenko

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

Or rather, *I think* it should truncate it (i.e. zero-extend to full width), but conceivably some CPUs can be buggy wrt that:
they can decide to modify only lower 16 bits of IP,
or even they can decided to use signed <offset16> but apply it to full-width RIP.

AMD manuals are not clear on what exactly should happen.

I am sure no one sane uses this form of branch instructions in 32-bit and 64-bit code.

Quoted 4 lines

I don't think we should be trying to support it "correctly" (we can just let program crash with SIGILL or something), we only need to make sure we don't overlook its existence and thus are not tricked into touching or modifying unrelated data.

And after the quick check it seems that lib/insn.c doesn't parse "66 e8 nn nn" correctly. It treats the next 2 bytes as the part of 32bit offset.

Quoted 19 lines · Oleg Nesterov

Quoted 18 lines · Denys Vlasenko

There is this monstrosity, "16-bit override for branches" in 64-mode:

66 e8 nn nn callw <offset16>

Nobody sane uses it because it truncates instruction pointer.

Or rather, *I think* it should truncate it (i.e. zero-extend to full width), but conceivably some CPUs can be buggy wrt that:
they can decide to modify only lower 16 bits of IP,
or even they can decided to use signed <offset16> but apply it to full-width RIP.

AMD manuals are not clear on what exactly should happen.

I am sure no one sane uses this form of branch instructions in 32-bit and 64-bit code.

Quoted 8 lines

Quoted 4 lines

I don't think we should be trying to support it "correctly" (we can just let program crash with SIGILL or something), we only need to make sure we don't overlook its existence and thus are not tricked into touching or modifying unrelated data.

And after the quick check it seems that lib/insn.c doesn't parse "66 e8 nn nn" correctly. It treats the next 2 bytes as the part of 32bit offset.

I didn't run-test it yet. By code inspection, it seems to work...

x86-opcode-map.txt:

    e8: CALL Jz (f64)

gen-insn-attr-x86.awk:

    imm_flag["Jz"] = "INAT_MAKE_IMM(INAT_IMM_VWORD32)"

insn.c:

        case INAT_IMM_VWORD32:
                if (!__get_immv32(insn))
                        goto err_out;
...
static int __get_immv32(struct insn *insn)
{
        switch (insn->opnd_bytes) {
        case 2:
                insn->immediate.value = get_next(short, insn);
                insn->immediate.nbytes = 2;
                break;
        case 4:
        case 8:
                insn->immediate.value = get_next(int, insn);
                insn->immediate.nbytes = 4;
                break;

...until I notice this code:

void insn_get_modrm(struct insn *insn)
{
...

        if (insn->x86_64 && inat_is_force64(insn->attr))
                insn->opnd_bytes = 8;

The (f64) modifier in x86-opcode-map.txt means that inat_is_force64() is true for call opcode. So we won't reach "case 2:" in __get_immv32():
insn_get_prefixes() did set insn->opnd_bytes to 2 when it saw 0x66 prefix, but it was before we reach this place, and here we overrode it. This is a bug in insn decoder.

Quoted 5 lines

The (f64) modifier in x86-opcode-map.txt means that inat_is_force64() is true for call opcode. So we won't reach "case 2:" in __get_immv32():
insn_get_prefixes() did set insn->opnd_bytes to 2 when it saw 0x66 prefix, but it was before we reach this place, and here we overrode it. This is a bug in insn decoder.

I tested it on both Intel and AMD CPUs and my worst fears came true:
this instruction has different widths on different CPUs.

This program:

# compile with: gcc -nostartfiles -nostdlib -o int3 int3.S
_start:         .globl  _start
                .byte 0x66,0xe9,0,0
                .byte 0,0
1: jmp 1b

compiles to this:

00000000004000b0 <_start>:

  4000b0:       66 e9 00 00             jmpw   b4 <_start-0x3ffffc>
  4000b4:       00 00                   add    %al,(%rax)
  4000b6:       eb fe                   jmp    4000b6 <_start+0x6>

and it will reach 0x4000b6 on Intel CPU.
IOW, Intel SandyBridge CPU thinks that insn is in fact 66 e9 00 00 00 00, no RIP truncation occurs.

On AMD K10 CPU, the very same binary jumps to 0x00b4 and gets SIGSEGV with MAPERR.
AMD thinks that the insn is 66 e9 00 00 as shown above.

Thus, insn.c decoder implements Intel's idea of this insn while binutils (objdump, gdb) implement AMD decode.

This same program can be compiled to 32-bit code,
in this mode both CPUs treat insn as 66 e9 00 00.

Oleg, I'm sure you are very sympathetic by now to the idea of just not supporting this insn at all. ;)

You can check whether insn had any prefix by checking insn->prefixes->nbytes != 0...

..but there is a problem with that. P4 introduced branch hints, which are implemented using segment prefixes on conditional jumps. Meaning that some compilers produce

2e 0f 82 nn nn nn nn

as (hint not taken) JB <offset32> insn.

2e is CS segment prefix. insn->prefixes->nbytes == 1 for this insn. DS prefix (3e) hints that branch is taken.

They were nearly useless on P4 anyway and ignored by all CPUs before or since, but they can be seen in some programs.

Looks like we'll need this:

/*
* 16-bit overrides such as CALLW (66 e8 nn nn) are not supported.
* Intel and AMD behavior differ in 64-bit mode: Intel ignores 66 prefix.
* No one uses these insns.
* To filter them out, reject any branch insns with prefixes... */
if (insn->prefixes->nbytes > 1)

	bail_out;

/*
* ...Except a single 3e or 2e "branch taken/not taken" hint prefix.
* These are (rarely) used, but ignored by any CPU except P4.
* Example: 2e 0f 82 nn nn nn nn is JB,PN <offset32>
*/
if (insn->prefixes->nbytes == 1 && (insn->prefixes->bytes[0] | 0x10) != 0x3e))

	bail_out;

Quoted 2 lines

You can check whether insn had any prefix by checking insn->prefixes->nbytes != 0...

No, since there are other prefixes (and it may be meaningless) you should find 0x66 in insn->prefixes->bytes[].

Quoted 5 lines · Masami Hiramatsu

Quoted 2 lines

You can check whether insn had any prefix by checking insn->prefixes->nbytes != 0...

No, since there are other prefixes (and it may be meaningless) you should find 0x66 in insn->prefixes->bytes[].

What "no"? Is my statement not true? Please reread it. I'm not talking about checking for 0x66 prefix. Here, I am talking about checking that insn has *any* prefix.

Incomplete, lacks "jcxz". Simple to fix. Anything else?

Quoted 2 lines

Incomplete, lacks "jcxz". Simple to fix. Anything else?

Please see v2 below. Simplify the preprocessor hacks.

Quoted 2 lines · Oleg Nesterov

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 <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/uprobes.c |   61 +++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 59 insertions(+), 2 deletions(-)

arch/x86/kernel/uprobes.c +5 -0

index 9283024..3865d8b 100644
@@ -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))				\

arch/x86/kernel/uprobes.c +30 -0

+
+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();
+	}

arch/x86/kernel/uprobes.c +7 -2

 	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;

Quoted 10 lines · Oleg Nesterov

arch/x86/kernel/uprobes.c +9 -0

+#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))

arch/x86/kernel/uprobes.c static bool ttt_is_call(struct arch_uprobe *auprobe) · line 483

Quoted 6 lines

arch/x86/kernel/uprobes.c +6 -0

+
+#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().

arch/x86/kernel/uprobes.c static bool ttt_is_call(struct arch_uprobe *auprobe) · line 523

Quoted 4 lines

static bool ttt_emulate_op(struct arch_uprobe *auprobe, struct pt_regs *regs) {

arch/x86/kernel/uprobes.c +1 -0

 	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.

But I'll try to cleanup this patch...

Quoted 2 lines · Oleg Nesterov

But I'll try to cleanup this patch...

See v2 below.

-------------------------------------------------------------------------------
Subject: [RFC PATCH v2 6/6] uprobes/x86: Emulate rip-relative conditional "near" jmp's

It seems that 16bit condi jmp is just 0x0f + short_jump_opc_plus_0x10.

Reported-by Jonathan Lebon <[email protected]>

Signed-off-by Oleg Nesterov <[email protected]>

---
 arch/x86/kernel/uprobes.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

arch/x86/kernel/uprobes.c +0 -0

index 3865d8b..dae02f9 100644

Quoted 2 lines · Oleg Nesterov

It seems that 16bit condi jmp is just 0x0f + short_jump_opc_plus_0x10.

Yes, but the code could use a comment to that effect. See below.

Searching for "jump" in the AMD manual, I see that there are 3 other instructions that are essentially conditional branches: loop, loope/loopz, and loopne/loopnz.

The offset is always 8 bits.

BTW, patches 2 and 3 look fine to me.

Quoted 6 lines · Jim Keniston

Quoted 3 lines · Oleg Nesterov

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

Quoted 1 line

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

Works! Thanks a lot.

Still I can't understand why displacement.nbytes == 0 in this case... Nevermind.

OK. Please see the RFC changes. Obviously not for inclusion yet. And totally untested, except I verified that the test-case from 4/6 works.

Please comment.

Oleg.

Quoted 6 lines · Jim Keniston

Quoted 3 lines · Oleg Nesterov

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

Quoted 1 line

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

Works! Thanks a lot.

Still I can't understand why displacement.nbytes == 0 in this case... Nevermind.

OK. Please see the RFC changes. Obviously not for inclusion yet. And totally untested, except I verified that the test-case from 4/6 works.

Please comment.

Oleg.