kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

An altenative solution would be to check bits 24 and 22 to see whether the instruction actually is a write back instruction or not. I think it's enough to check whether Rn != PC, because:
- it's looks cheaper than the alternative
- to my understaning it's not permitted to update the PC with a write back instruction, so we don't lose any ability to emulate legal instructions.
- in case of writing back for non write back instructions where Rn != PC, it doesn't matter because the values are the same.

Regarding the second point above, it would possibly be prudent to add some checking to prep_emulate_ldr_str(), so that instructions with write back and Rn == PC would be rejected.

Quoted 7 lines · Viktor Rosendahl

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

It looks like we have the same problem with emulate_ldrd and emulate_strd as well.

Quoted 10 lines · ext Tixy

Quoted 7 lines · Viktor Rosendahl

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

It looks like we have the same problem with emulate_ldrd and emulate_strd as well.

Yes, it seems so. Currently emulate_ldrd and emulate_strd don't even have the adjustment of the PC value, so in case of Rn == PC, it will not update the PC incorrectly but instead load/store from the wrong address. Below is a patch that adds both the adjustment of the PC value and the check for PC == PC.

I haven't tested this code at all and I am not sure of how to test it.

Signed-off-by Viktor Rosendahl <[email protected]>

---
 arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

arch/arm/kernel/kprobes-decode.c +15 -5

index 2389131..3b0cf90 100644
@@ -540,9 +540,12 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 {
 	insn_2arg_fn_t *i_fn = (insn_2arg_fn_t *)&p->ainsn.insn[0];
 	kprobe_opcode_t insn = p->opcode;
+	long ppc = (long)p->addr + 8;
 	int rd = (insn >> 12) & 0xf;
 	int rn = (insn >> 16) & 0xf;
 	int rm = insn & 0xf;  /* rm may be invalid, don't care. */
+	long rmv = (rm == 15) ? ppc : regs->uregs[rm];
+	long rnv = (rn == 15) ? ppc : regs->uregs[rn];
 
 	/* Not following the C calling convention here, so need asm(). */
 	__asm__ __volatile__ (

@@ -554,29 +557,36 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 		"str	r0, %[rn]	\n\t"	/* in case of writeback */
 		"str	r2, %[rd0]	\n\t"
 		"str	r3, %[rd1]	\n\t"
-		: [rn]  "+m" (regs->uregs[rn]),
+		: [rn]  "+m" (rnv),
 		  [rd0] "=m" (regs->uregs[rd]),
 		  [rd1] "=m" (regs->uregs[rd+1])
-		: [rm]   "m" (regs->uregs[rm]),
+		: [rm]   "m" (rmv),
 		  [cpsr] "r" (regs->ARM_cpsr),
 		  [i_fn] "r" (i_fn)
 		: "r0", "r1", "r2", "r3", "lr", "cc"
 	);
+	if (rn != 15)
+		regs->uregs[rn] = rnv;  /* Save Rn in case of writeback. */
 }
 
 static void __kprobes emulate_strd(struct kprobe *p, struct pt_regs *regs)
 {
 	insn_4arg_fn_t *i_fn = (insn_4arg_fn_t *)&p->ainsn.insn[0];
 	kprobe_opcode_t insn = p->opcode;
+	long ppc = (long)p->addr + 8;
 	int rd = (insn >> 12) & 0xf;
 	int rn = (insn >> 16) & 0xf;
 	int rm  = insn & 0xf;
-	long rnv = regs->uregs[rn];
-	long rmv = regs->uregs[rm];  /* rm/rmv may be invalid, don't care. */
+	long rnv = (rn == 15) ? ppc : regs->uregs[rn];
+	/* rm/rmv may be invalid, don't care. */
+	long rmv = (rm == 15) ? ppc : regs->uregs[rm];
+	long rnv_wb;
 
-	regs->uregs[rn] = insnslot_4arg_rflags(rnv, rmv, regs->uregs[rd],
+	rnv_wb = insnslot_4arg_rflags(rnv, rmv, regs->uregs[rd],
 					       regs->uregs[rd+1],
 					       regs->ARM_cpsr, i_fn);
+	if (rn != 15)
+		regs->uregs[rn] = rnv_wb;  /* Save Rn in case of writeback. */
 }
 
 static void __kprobes emulate_ldr(struct kprobe *p, struct pt_regs *regs)

Signature · 1 line

1.7.2.5

Please send to RMK's patch system.

I agree that it might be a better idea to simply reject the dubious cases upfront from arm_kprobe_decode_insn() and keep the actual

emulation code fast and free from odd conditions that might never be useful in practice. I think this is highly unlikely that we would find some usage of LDRD/STRD indexed by r15 in the kernel.

Quoted 2 lines · ext Nicolas Pitre

I agree that it might be a better idea to simply reject the dubious cases upfront from arm_kprobe_decode_insn() and keep the actual

What do you mean by "dubious cases" ?

Do you mean oddball special cases of instructions that are fully legal with a well defined behavior, although they are unlikely to be emitted by gcc ?

..or do you mean instructions whose behavior are undefined by the current architecture but would not necessarily cause an illegal instruction exception ?

My take is that it could be worth checking for as many as possible of the legal oddball cases. When it comes to instructions with undefined behavior, I think the ideal would be if they are rejected by arm_kprobe_decode_insn().

My guess is that most of the kprobe slowdown will not anyway come from a few extra checks in the emulation/simulation code but from the handling of the illegal instruction exceptions that will occur when the probe is hit and at the end of single stepping.

Quoted 3 lines

I think this is highly unlikely that we would find some usage of LDRD/STRD indexed by r15 in the kernel.

I guess that depends on the gcc backend. When doing an "objdump -d vmlinux", I found this:

b00165fc <sort_main_extable>:
b00165fc: e59f0004 ldr r0, [pc, #4] ; b0016608
b0016600: e59f1004 ldr r1, [pc, #4] ; b001660c
b0016604: ea074262 b b01e6f94 <sort_extable>
b0016608: b0548840 .word 0xb0548840
b001660c: b0549770 .word 0xb0549770

Now, I admit that it's possible that somewhere beyond the horizon of my understanding there is some good reason to do two LDRs into adjacent registers from adjacent memory addresses, instead of merging them into one LDRD.

However, it gives me the impression that it would not be that unlikely that some future version of gcc could generate an LDRD in some function prologues.

BTW, in my kernel, LDR indexed by r15 is a really common instruction at the very beginning of functions. I am not sure why; it could have something to do with the fact that the kernel is compiled without frame pointers.

Sorry, I meant r15-indexed with a write back.

Quoted 1 line · Nicolas Pitre

Sorry, I meant r15-indexed with a write back.

I believe that's unpredictable. So actually you can make kprobes do anything you like with it - no one should ever generate an instruction which read-modify-writes the PC.

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line · Nicolas Pitre

Sorry, I meant r15-indexed with a write back.

Quoted 3 lines

I believe that's unpredictable. So actually you can make kprobes do anything you like with it - no one should ever generate an instruction which read-modify-writes the PC.

Indeed. Hence my suggestion to simply refuse and abort the placement of a probe on such instructions and keep the actual emulation code free of tests for those odd cases. In practice this shouldn't affect anyone.

Quoted 5 lines · ext Nicolas Pitre

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line · Nicolas Pitre

Sorry, I meant r15-indexed with a write back.

Quoted 8 lines

Quoted 3 lines

I believe that's unpredictable. So actually you can make kprobes do anything you like with it - no one should ever generate an instruction which read-modify-writes the PC.

Indeed. Hence my suggestion to simply refuse and abort the placement of a probe on such instructions and keep the actual emulation code free of tests for those odd cases. In practice this shouldn't affect anyone.

Here is a patch for implementing the rejection of probes on those instructions, with Rn == 15 and writeback enabled. Those previous patches are still required, since they fix the emulation of the fully legal instructions where Rn == 15 and writeback isn't enabled.

If we start littering the code with all these extra checks we risk introducing bugs and making the code more difficult to maintain.

In my opinion we should not add any extra code to handle instructions combinations that the ARM ARM says are UNPREDICTABLE, or have fields which are SBZ/SBO. The toolchain shouldn't ever generate these bad instructions in which case the extra kprobes code is redundant.

Quoted 8 lines · ext Tixy

If we start littering the code with all these extra checks we risk introducing bugs and making the code more difficult to maintain.

In my opinion we should not add any extra code to handle instructions combinations that the ARM ARM says are UNPREDICTABLE, or have fields which are SBZ/SBO. The toolchain shouldn't ever generate these bad instructions in which case the extra kprobes code is redundant.

I see your point. I guess we can decide to not care about those unpredictable cases, unless someone can come up with some decoding & checking code that covers all the cases and is easy to understand and maintain.

Quoted 15 lines · Viktor Rosendahl

Quoted 5 lines · ext Nicolas Pitre

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line · Nicolas Pitre

Sorry, I meant r15-indexed with a write back.

Quoted 8 lines

Quoted 3 lines

I believe that's unpredictable. So actually you can make kprobes do anything you like with it - no one should ever generate an instruction which read-modify-writes the PC.

Indeed. Hence my suggestion to simply refuse and abort the placement of a probe on such instructions and keep the actual emulation code free of tests for those odd cases. In practice this shouldn't affect anyone.

Quoted 4 lines

Here is a patch for implementing the rejection of probes on those instructions, with Rn == 15 and writeback enabled. Those previous patches are still required, since they fix the emulation of the fully legal instructions where Rn == 15 and writeback isn't enabled.

I think this could be a slippery slope, what about the other dubious combinations, like writeback when Rn==Rt, or when Rm==pc? By the same rationale we should check for those to.

If we start littering the code with all these extra checks we risk introducing bugs and making the code more difficult to maintain.

In my opinion we should not add any extra code to handle instructions combinations that the ARM ARM says are UNPREDICTABLE, or have fields which are SBZ/SBO. The toolchain shouldn't ever generate these bad instructions in which case the extra kprobes code is redundant.

Quoted 25 lines · Tixy

Quoted 15 lines · Viktor Rosendahl

Quoted 5 lines · ext Nicolas Pitre

Quoted 3 lines · Russell King - ARM Linux

Quoted 1 line · Nicolas Pitre

Sorry, I meant r15-indexed with a write back.

Quoted 8 lines

Quoted 3 lines

I believe that's unpredictable. So actually you can make kprobes do anything you like with it - no one should ever generate an instruction which read-modify-writes the PC.

Indeed. Hence my suggestion to simply refuse and abort the placement of a probe on such instructions and keep the actual emulation code free of tests for those odd cases. In practice this shouldn't affect anyone.

Quoted 4 lines

Here is a patch for implementing the rejection of probes on those instructions, with Rn == 15 and writeback enabled. Those previous patches are still required, since they fix the emulation of the fully legal instructions where Rn == 15 and writeback isn't enabled.

I think this could be a slippery slope, what about the other dubious combinations, like writeback when Rn==Rt, or when Rm==pc? By the same rationale we should check for those to.

Quoted 7 lines

If we start littering the code with all these extra checks we risk introducing bugs and making the code more difficult to maintain.

In my opinion we should not add any extra code to handle instructions combinations that the ARM ARM says are UNPREDICTABLE, or have fields which are SBZ/SBO. The toolchain shouldn't ever generate these bad instructions in which case the extra kprobes code is redundant.

There are two categories to consider for such issues:

1) What to do with any writeback to pc. This is really important because this directly affects the running thread and determines where execution will resume once the probe is done. Any dubious case should be carefully intercepted, otherwise this may become a potential security risk. So, given that this has to be handled somehow, I think it is best to simply refuse the placement of a probe on a dubious instruction rather than working around the dubiousness in the actual emulation code.

2) What to do with any other undefined combinations. Well, anything else that is defined as unpredictable by the ARM ARM which doesn't involve writing the pc should simply pass through the emulation code without incuring any additional overhead or special care. By definition, if the result is unpredictable, we can emulate it the way we wish and any discrepancy with native hardware result is unimportant.

In the first category can be found things like:

	ldmdb	pc!, {r0, r1, r2}

In the second category would be:

	ldmdb	pc, {r0, r1, r2}

We can safely ignore the second case, but the first case clearly has the potential for trouble if mis-emulated. And trying to correctly emulate any of those cases is worthless.

Quoted 3 lines · Nicolas Pitre

We can safely ignore the second case, but the first case clearly has the potential for trouble if mis-emulated. And trying to correctly emulate any of those cases is worthless.

I don't think it's quite as black and white. In both cases the kernel was executing an illegal instruction before we inserted the probe, so we are probably already in the territorial of "affects the running thread" and security issues.

However, if the simple rule we need to follow is "avoid ambiguous writes to PC" then I won't put up any more fight :-) (Expecially as I just checked normal execution of one writeback to PC instruction an found that PC was unaffected ;-)

Sorry, I meant r15-indexed with a write back.

Quoted 3 lines

Now, I admit that it's possible that somewhere beyond the horizon of my understanding there is some good reason to do two LDRs into adjacent registers from adjacent memory addresses, instead of merging them into one LDRD.

In this case I suspect that the loaded values were pushed to the literal pool, and it is hard for the compiler to ensure the placement is always 64-bit aligned.

Quoted 3 lines

BTW, in my kernel, LDR indexed by r15 is a really common instruction at the very beginning of functions. I am not sure why; it could have something to do with the fact that the kernel is compiled without frame pointers.

No, it's all about literal pool usage.

But nowhere will you find pc-indexed addressing with a writeback. That's one of the cases I think should be rejected upfront instead of evaluating this possibility which is likely to never happen in practice each time the instruction is emulated.

Quoted 10 lines · ext Tixy

Quoted 7 lines · Viktor Rosendahl

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

It looks like we have the same problem with emulate_ldrd and emulate_strd as well.

Yes, it seems so. Currently emulate_ldrd and emulate_strd don't even have the adjustment of the PC value, so in case of Rn == PC, it will not update the PC incorrectly but instead load/store from the wrong address. Below is a patch that adds both the adjustment of the PC value and the check for PC == PC.

I haven't tested this code at all and I am not sure of how to test it.

Signed-off-by Viktor Rosendahl <[email protected]>

---
 arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

arch/arm/kernel/kprobes-decode.c +4 -1

index 2389131..3b0cf90 100644
@@ -540,9 +540,12 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 {
 	insn_2arg_fn_t *i_fn = (insn_2arg_fn_t *)&p->ainsn.insn[0];
 	kprobe_opcode_t insn = p->opcode;
+	long ppc = (long)p->addr + 8;
 	int rd = (insn >> 12) & 0xf;
 	int rn = (insn >> 16) & 0xf;
 	int rm = insn & 0xf;  /* rm may be invalid, don't care. */
+	long rmv = (rm == 15) ? ppc : regs->uregs[rm];
+	long rnv = (rn == 15) ? ppc : regs->uregs[rn];
 
 	/* Not following the C calling convention here, so need asm(). */
 	__asm__ __volatile__ (

@@ -554,29 +557,36 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 		"str	r0, %[rn]	\n\t"	/* in case of writeback */
 		"str	r2, %[rd0]	\n\t"
 		"str	r3, %[rd1]	\n\t"
-		: [rn]  "+m" (regs->uregs[rn]),
+		: [rn]  "+m" (rnv),
 		  [rd0] "=m" (regs->uregs[rd]),

arch/arm/kernel/kprobes-decode.c +8 -3

 static void __kprobes emulate_strd(struct kprobe *p, struct pt_regs *regs)
 {
 	insn_4arg_fn_t *i_fn = (insn_4arg_fn_t *)&p->ainsn.insn[0];
 	kprobe_opcode_t insn = p->opcode;
+	long ppc = (long)p->addr + 8;
 	int rd = (insn >> 12) & 0xf;
 	int rn = (insn >> 16) & 0xf;
 	int rm  = insn & 0xf;
-	long rnv = regs->uregs[rn];
-	long rmv = regs->uregs[rm];  /* rm/rmv may be invalid, don't care. */
+	long rnv = (rn == 15) ? ppc : regs->uregs[rn];
+	/* rm/rmv may be invalid, don't care. */
+	long rmv = (rm == 15) ? ppc : regs->uregs[rm];
+	long rnv_wb;
 
-	regs->uregs[rn] = insnslot_4arg_rflags(rnv, rmv, regs->uregs[rd],
+	rnv_wb = insnslot_4arg_rflags(rnv, rmv, regs->uregs[rd],
 					       regs->uregs[rd+1],
 					       regs->ARM_cpsr, i_fn);
+	if (rn != 15)
+		regs->uregs[rn] = rnv_wb;  /* Save Rn in case of writeback. */
 }
 
 static void __kprobes emulate_ldr(struct kprobe *p, struct pt_regs *regs)

From looking at them, the changes look functionally correct though. I do have a suggestion for slight change, see inline comments below...

If the variables rnv and rmv were declared as registers then we could avoid 3 stores and 3 loads of stack values. This would require changing the the assembler as well, e.g. something like this...

	register long rnv asm("r0");
	register long rmv asm("r1");
	rnv = (rn == 15) ? ppc : regs->uregs[rn];
	rmv = (rm == 15) ? ppc : regs->uregs[rm];

	__asm__ __volatile__ (
		"msr	cpsr_fs, %[cpsr]\n\t"
		"mov	lr, pc		\n\t"
		"mov	pc, %[i_fn]	\n\t"
		"str	r2, %[rd0]	\n\t"
		"str	r3, %[rd1]	\n\t"
		:       "=r" (rnv),
		  [rd0] "=m" (regs->uregs[rd]),
		  [rd1] "=m" (regs->uregs[rd+1])

		:        "0" (rnv),
		         "r" (rmv),
		  [cpsr] "r" (regs->ARM_cpsr),
		  [i_fn] "r" (i_fn)
		: "r2", "r3", "lr", "cc"
  	);
	if (rn != 15)
		regs->uregs[rn] = rnv;

My patch was about fixing the correctness of the emulation regarding the use of the PC register as Rn. To my understanding, this is a performance optimization that will optimize the register/stack usage.

I am not opposed to this in any way but I really think that fixing the correctness and optimizing the performance should be in separate patches. Also, I already submitted my patch to RMK's patch system, so you are a bit late :)

Quoted 12 lines · Viktor Rosendahl

Quoted 10 lines · ext Tixy

Quoted 7 lines · Viktor Rosendahl

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

It looks like we have the same problem with emulate_ldrd and emulate_strd as well.

Quoted 6 lines

Yes, it seems so. Currently emulate_ldrd and emulate_strd don't even have the adjustment of the PC value, so in case of Rn == PC, it will not update the PC incorrectly but instead load/store from the wrong address. Below is a patch that adds both the adjustment of the PC value and the check for PC == PC.

I haven't tested this code at all and I am not sure of how to test it.

In a day or two I should have support in my test code for these sorts of instructions. From looking at them, the changes look functionally correct though. I do have a suggestion for slight change, see inline comments below...

Quoted 30 lines

Signed-off-by Viktor Rosendahl <[email protected]>

---
 arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

arch/arm/kernel/kprobes-decode.c +4 -1

index 2389131..3b0cf90 100644
@@ -540,9 +540,12 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 {
 	insn_2arg_fn_t *i_fn = (insn_2arg_fn_t *)&p->ainsn.insn[0];
 	kprobe_opcode_t insn = p->opcode;
+	long ppc = (long)p->addr + 8;
 	int rd = (insn >> 12) & 0xf;
 	int rn = (insn >> 16) & 0xf;
 	int rm = insn & 0xf;  /* rm may be invalid, don't care. */
+	long rmv = (rm == 15) ? ppc : regs->uregs[rm];
+	long rnv = (rn == 15) ? ppc : regs->uregs[rn];
 
 	/* Not following the C calling convention here, so need asm(). */
 	__asm__ __volatile__ (

@@ -554,29 +557,36 @@ static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs)
 		"str	r0, %[rn]	\n\t"	/* in case of writeback */
 		"str	r2, %[rd0]	\n\t"
 		"str	r3, %[rd1]	\n\t"
-		: [rn]  "+m" (regs->uregs[rn]),
+		: [rn]  "+m" (rnv),
 		  [rd0] "=m" (regs->uregs[rd]),

arch/arm/kernel/kprobes-decode.c static void __kprobes emulate_ldrd(struct kprobe *p, struct pt_regs *regs) · line 570

Quoted 10 lines

arch/arm/kernel/kprobes-decode.c +3 -1

 		  [rd1] "=m" (regs->uregs[rd+1])
-		: [rm]   "m" (regs->uregs[rm]),
+		: [rm]   "m" (rmv),
 		  [cpsr] "r" (regs->ARM_cpsr),
 		  [i_fn] "r" (i_fn)
 		: "r0", "r1", "r2", "r3", "lr", "cc"
 	);
+	if (rn != 15)
+		regs->uregs[rn] = rnv;  /* Save Rn in case of writeback. */
 }

If the variables rnv and rmv were declared as registers then we could avoid 3 stores and 3 loads of stack values. This would require changing the the assembler as well, e.g. something like this...

	register long rnv asm("r0");
	register long rmv asm("r1");
	rnv = (rn == 15) ? ppc : regs->uregs[rn];
	rmv = (rm == 15) ? ppc : regs->uregs[rm];
 
	__asm__ __volatile__ (
		"msr	cpsr_fs, %[cpsr]\n\t"
		"mov	lr, pc		\n\t"
		"mov	pc, %[i_fn]	\n\t"
		"str	r2, %[rd0]	\n\t"
		"str	r3, %[rd1]	\n\t"
		:       "=r" (rnv),
		  [rd0] "=m" (regs->uregs[rd]),
		  [rd1] "=m" (regs->uregs[rd+1])
		:        "0" (rnv),
		         "r" (rmv),
		  [cpsr] "r" (regs->ARM_cpsr),
		  [i_fn] "r" (i_fn)
		: "r2", "r3", "lr", "cc"

);

Quoted 4 lines

Regarding the second point above, it would possibly be prudent to add some checking to prep_emulate_ldr_str(), so that instructions with write back and Rn == PC would be rejected.

I don't think it is worth adding code to check for illegal instructions. The toolchain shouldn't generate them in the first place, and there are many places in the kprobe code which doesn't bother checking; there are even comments like "may be invalid, don't care".

I'm currently working on implementing Thumb support in kprobes and am writing test code as part of that. I planned on adding test cases for ARM so hopefully will catch a few more instruction emulation bugs (if there are any to be found).

On 03/25/2011 11:19 PM, ext Tixy wrote:

Quoted 4 lines

I don't think it is worth adding code to check for illegal instructions. The toolchain shouldn't generate them in the first place, and there are many places in the kprobe code which doesn't bother checking; there are even comments like "may be invalid, don't care".

I think those "may be invalid, don't care" comments mostly are about the Rm value, which isn't valid for some fully legal variants of the instruction, those instructions that have the immediate bit set. In that case the Rm value, will actually be part of an immediate and thus bogus. However, it will not impact the result of the emulation because the instruction will not read from the r2 register. It's enough to check the immediate bit in the prep_emulate_*() functions; if you check for example the prep_emulate_ldr_str() function you will se that it actually does it before adjusting Rm to r2.

To summarize, I think the "may be invalid, don't care" comments simply mean "This value may be bogus but in that case it will not impact the result of the emulation so we don't care".

The Rn value from the emulation is unconditionally written back; this is fine as long as Rn != PC because in that case, even if the instruction isn't a write back instruction, it will only result in the same value being written back.

In case Rn == PC, then the emulated instruction doesn't have the actual PC value in Rn but an adjusted value; when this is written back, it will result in the PC being incorrectly updated.

An altenative solution would be to check bits 24 and 22 to see whether the instruction actually is a write back instruction or not. I think it's enough to check whether Rn != PC, because:
- it's looks cheaper than the alternative
- to my understaning it's not permitted to update the PC with a write back instruction, so we don't lose any ability to emulate legal instructions.
- in case of writing back for non write back instructions where Rn != PC, it doesn't matter because the values are the same.

Regarding the second point above, it would possibly be prudent to add some checking to prep_emulate_ldr_str(), so that instructions with write back and Rn == PC would be rejected.

Signed-off-by Viktor Rosendahl <[email protected]>

---
 arch/arm/kernel/kprobes-decode.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

arch/arm/kernel/kprobes-decode.c +6 -4

index 8f6ed43..2389131 100644
@@ -594,7 +594,8 @@ static void __kprobes emulate_ldr(struct kprobe *p, struct pt_regs *regs)
 	long cpsr = regs->ARM_cpsr;
 
 	fnr.dr = insnslot_llret_3arg_rflags(rnv, 0, rmv, cpsr, i_fn);
-	regs->uregs[rn] = fnr.r0;  /* Save Rn in case of writeback. */
+	if (rn != 15)
+		regs->uregs[rn] = fnr.r0;  /* Save Rn in case of writeback. */
 	rdv = fnr.r1;
 
 	if (rd == 15) {

@@ -622,10 +623,11 @@ static void __kprobes emulate_str(struct kprobe *p, struct pt_regs *regs)
 	long rdv = (rd == 15) ? iaddr + str_pc_offset : regs->uregs[rd];
 	long rnv = (rn == 15) ? iaddr +  8 : regs->uregs[rn];
 	long rmv = regs->uregs[rm];  /* rm/rmv may be invalid, don't care. */
+	long rnv_wb;
 
-	/* Save Rn in case of writeback. */
-	regs->uregs[rn] =
-		insnslot_3arg_rflags(rnv, rdv, rmv, regs->ARM_cpsr, i_fn);
+	rnv_wb = insnslot_3arg_rflags(rnv, rdv, rmv, regs->ARM_cpsr, i_fn);
+	if (rn != 15)
+		regs->uregs[rn] = rnv_wb;  /* Save Rn in case of writeback. */
 }
 
 static void __kprobes emulate_mrrc(struct kprobe *p, struct pt_regs *regs)

Signature · 7 lines

1.7.2.5


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel