This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: uprobes and empty functions


On 11/01/2010 11:42 AM, Jim Keniston wrote:
> On Mon, 2010-11-01 at 10:42 -0700, Josh Stone wrote:
>> The problem is that we're validating the prefixes separately from the
>> actual opcodes, without being sure that every combination works.
>>
>> I don't think we need to bother with asking what are "reasonable"
>> prefixes on *every* opcode -- in truly illegal cases, the cpu will fault
>> and uprobes should properly catch and clean up after.  But on opcodes
>> for which we have any kind of manual fixup or emulation, we need to be
>> certain that we can handle prefixes as well, or else refuse to probe.
> 
> We do post-single-step fixups on almost all instructions -- typically
> just redirecting the IP back to the probed instruction stream.  ret is
> one of the few instruction types where we figured we DON'T need to do a
> fixup.

Noted - I guess I meant those "special" opcodes where the fixup is
something other than moving a relative IP back to the instruction
stream, i.e. everything in uprobe_post_ssout's switch-statement.  For
those, we either need to recognize the prefix or refuse to deal with it.

> The difficulty in testing {k,u}probes's handling of every
> prefix/instruction combo is that we have to verify that the probed
> instruction did "the right thing" -- i.e., what the unprobed instruction
> would do.  Realistically, that means testing only instructions in
> executables that produce predictable results.  With kprobes, you at
> least have a well-defined set of instructions that need to be tested --
> i.e., all the instructions in the kernel and modules -- even if you
> can't arrange to execute them all.  With uprobes, the set of executables
> we have to probe is unbounded.

I *think* for all other "non-special" opcodes, we can assume either:
- combo is invalid, and SSOL will fault (which uprobes can deal with)
- combo is valid, and the normal relative-IP fixup is fine.

Are there exceptions to this?

>> I'll try enhancing my patch to restrict this, and post it shortly...

Attached.  I'm refusing to probe any of those "special" instructions if
they have a prefix, with the sole exception of "rep ret" because we can
make sense of that.  (I only diffed uprobes2 this time, but of course
the others need the same fix...)


Josh
diff --git a/runtime/uprobes2/uprobes_x86.c b/runtime/uprobes2/uprobes_x86.c
index 02ec76b..d377942 100644
--- a/runtime/uprobes2/uprobes_x86.c
+++ b/runtime/uprobes2/uprobes_x86.c
@@ -197,10 +197,54 @@ static void report_bad_2byte_opcode(uprobe_opcode_t op)
 		"instructions with the 2-byte opcode 0x0f 0x%2.2x\n", op);
 }
 
+/* Check that uprobe_post_ssout() can deal with this insn's prefix */
+static int validate_ssout_prefix(struct uprobe_probept *ppt,
+		uprobe_opcode_t *insn)
+{
+	uprobe_opcode_t *prefix = ppt->insn;
+	unsigned prefix_size = insn - prefix;
+
+	if (prefix_size == 0)
+		return 0; /* no prefix, so it's ok. */
+
+	/*
+	 * Basically, this just returns -EPERM for every instruction where
+	 * uprobe_post_ssout is currently assuming there's no prefix.
+	 */
+	switch (*insn) {
+	case 0xc3:		/* ret/lret */
+		if (prefix_size == 1 && *prefix == 0xf3)
+			return 0; /* rep-ret is explicitly allowed */
+		return -EPERM;
+
+	case 0xcb:		/* more ret/lret */
+	case 0xc2:
+	case 0xca:
+		return -EPERM;
+	case 0xe8:		/* call relative - Fix return addr */
+		return -EPERM;
+	case 0x9a:		/* call absolute - Fix return addr */
+		return -EPERM;
+	case 0xff:
+		if ((insn[1] & 0x30) == 0x10) {
+			/* call absolute, indirect */
+			return -EPERM;
+		} else if ((insn[1] & 0x31) == 0x20 ||	/* jmp near, absolute indirect */
+			   (insn[1] & 0x31) == 0x21) {	/* jmp far, absolute indirect */
+			return -EPERM;
+		}
+		break;
+	case 0xea:		/* jmp absolute -- rip is correct */
+		return -EPERM;
+	}
+
+	return 0;
+}
+
 static int validate_insn_32bits(struct uprobe_probept *ppt)
 {
 	uprobe_opcode_t *insn = ppt->insn;
-	int pfx;
+	int pfx, ret;
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	while ((pfx = check_legacy_prefix(insn[0])) == 1)
@@ -209,6 +253,8 @@ static int validate_insn_32bits(struct uprobe_probept *ppt)
 		report_bad_1byte_opcode(32, insn[0]);
 		return -EPERM;
 	}
+        if ((ret = validate_ssout_prefix(ppt, insn)) != 0)
+            return ret;
 	if (test_bit(insn[0], (unsigned long*)good_insns_32))
 		return 0;
 	if (insn[0] == 0x0f) {
@@ -223,7 +269,7 @@ static int validate_insn_32bits(struct uprobe_probept *ppt)
 static int validate_insn_64bits(struct uprobe_probept *ppt)
 {
 	uprobe_opcode_t *insn = ppt->insn;
-	int pfx;
+	int pfx, ret;
 
 	/* Skip good instruction prefixes; reject "bad" ones. */
 	while ((pfx = check_legacy_prefix(insn[0])) == 1)
@@ -235,6 +281,8 @@ static int validate_insn_64bits(struct uprobe_probept *ppt)
 	/* Skip REX prefix. */
 	if ((insn[0] & 0xf0) == 0x40)
 		insn++;
+        if ((ret = validate_ssout_prefix(ppt, insn)) != 0)
+            return ret;
 	if (test_bit(insn[0], (unsigned long*)good_insns_64))
 		return 0;
 	if (insn[0] == 0x0f) {
@@ -636,9 +684,17 @@ void uprobe_post_ssout(struct uprobe_task *utask, struct uprobe_probept *ppt,
 	 * We don't bother skipping prefixes here because none of the
 	 * instructions that require special treatment (other than
 	 * rip-relative instructions, handled above) involve prefixes.
+	 * XXX Keep this assertion in sync with validate_ssout_prefix()!
 	 */
 
 	switch (*insn) {
+	case 0xf3:
+		if (insn[1] != 0xc3)
+			break;
+		/*
+		 * "rep ret" is an AMD kludge that's used by GCC,
+		 * so we need to treat it like a normal ret.
+		 */
 	case 0xc3:		/* ret/lret */
 	case 0xcb:
 	case 0xc2:

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]