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

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

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

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 <viktor.rosendahl@nokia.com>
---
 arch/arm/kernel/kprobes-decode.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
index 8f6ed43..2389131 100644
--- a/arch/arm/kernel/kprobes-decode.c
+++ b/arch/arm/kernel/kprobes-decode.c
@@ -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)
-- 
1.7.2.5

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

[Tixy]

On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote:
> 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.

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

I agree that the check for Rn != PC seems simplest and sufficient.

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

-- 
Tixy

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Viktor Rosendahl]

On 03/25/2011 11:19 PM, ext Tixy wrote:
> On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote:
>> 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 <viktor.rosendahl@nokia.com>
---
 arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
 1 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
index 2389131..3b0cf90 100644
--- a/arch/arm/kernel/kprobes-decode.c
+++ b/arch/arm/kernel/kprobes-decode.c
@@ -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)
-- 
1.7.2.5

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Nicolas Pitre]

On Mon, 28 Mar 2011, Viktor Rosendahl wrote:

> On 03/25/2011 11:19 PM, ext Tixy wrote:
> > On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote:
> >> 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 <viktor.rosendahl@nokia.com>

Looks OK from inspection though.

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

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.

Same issue if Rd happens to be odd, or equal to r14.

> ---
>  arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
> index 2389131..3b0cf90 100644
> --- a/arch/arm/kernel/kprobes-decode.c
> +++ b/arch/arm/kernel/kprobes-decode.c
> @@ -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)
> -- 
> 1.7.2.5
> 

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Viktor Rosendahl]

On 03/29/2011 01:39 AM, ext Nicolas Pitre wrote:
>
> Please send to RMK's patch system.
>

OK, will do.

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

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

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.

best regards,

Viktor

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Nicolas Pitre]

On Tue, 29 Mar 2011, Viktor Rosendahl wrote:

> On 03/29/2011 01:39 AM, ext Nicolas Pitre wrote:
> > 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
> ?

Probably both.  This code is already complex enough as it is now, so if 
unused conplexity can go then it'll be easier to make it efficient and 
bug free.  And since our target is the kernel itself then we know with a 
high degree of confidence what kind of instructions we have to deal 
with.

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

Yes.

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

Surely, but those extra checks havean implied maintenance cost too by 
making the code less obvious.

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

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

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

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

No, it's all about literal pool usage.  When you have to load the value 
0x12345678, it is cheaper to simply store the value out of line, and 
perform a relative load like this:

	ldr	r0, [pc, #_val - . - 8]
	...

_val:	.word	0x12345678

Sometimes, hand written assembly code would use this syntax:

	ldr	r0, =0x12345678

and the assembler will do the job of translating that into the above 
form automatically, or simply turn that into a "mov r0, #<value>" if the 
value is actually narrow enough to fit in the immediate constant 
constraint.

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.


Nicolas

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Russell King - ARM Linux]

On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
> 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.  Easiest and most efficient solution
is to make it work the same way as it would do with any other register -
in other words, by all means do the necessary correction to perform the
load, but allow the writeback to go ahead even if that means that the
resulting PC value is wrong.  It doesn't matter as you've actually
implemented what is required.

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Nicolas Pitre]

On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:

> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
> > 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.

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.


Nicolas

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Viktor Rosendahl]

On 03/29/2011 09:44 PM, ext Nicolas Pitre wrote:
 > On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:
 >
 >> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
 >>> 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.
 >
 > 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.

Signed-off-by: Viktor Rosendahl <viktor.rosendahl@nokia.com>
---
 arch/arm/kernel/kprobes-decode.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
index 3b0cf90..b5c89c8 100644
--- a/arch/arm/kernel/kprobes-decode.c
+++ b/arch/arm/kernel/kprobes-decode.c
@@ -868,6 +868,15 @@ static enum kprobe_insn __kprobes
 prep_emulate_ldr_str(kprobe_opcode_t insn, struct arch_specific_insn *asi)
 {
 	int ibit = (insn & (1 << 26)) ? 25 : 22;
+	int is_preindexed = (insn >> 24) & 0x1;
+	int is_writeback = (insn >> 21) & 0x1;
+	int rn = (insn >> 16) & 0xf;
+
+	/* Writeback is always enabled in post-indexed mode. */
+	is_writeback = is_writeback || !is_preindexed;
+	/* writeback to PC is unpredictable, so reject it */
+	if (is_writeback && rn == 15)
+		return INSN_REJECTED;
 
 	insn &= 0xfff00fff;
 	insn |= 0x00001000;	/* Rn = r0, Rd = r1 */
@@ -1115,6 +1124,16 @@ space_cccc_000x(kprobe_opcode_t insn, struct arch_specific_insn *asi)
 			return prep_emulate_rd12rn16rm0_wflags(insn, asi);
 		} else if ((insn & 0x0e1000d0) == 0x00000d0) {
 			/* STRD/LDRD */
+			int is_preindexed = (insn >> 24) & 0x1;
+			int is_writeback = (insn >> 21) & 0x1;
+			int rn = (insn >> 16) & 0xf;
+
+			/* Writeback is always enabled in post-indexed mode. */
+			is_writeback = is_writeback || !is_preindexed;
+			/* writeback to PC is unpredictable, so reject it */
+			if (is_writeback && rn == 15)
+				return INSN_REJECTED;
+
 			insn &= 0xfff00fff;
 			insn |= 0x00002000;	/* Rn = r0, Rd = r2 */
 			if (insn & (1 << 22)) {
-- 
1.7.2.5

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Tixy]

On Wed, 2011-03-30 at 16:42 +0300, Viktor Rosendahl wrote:
> On 03/29/2011 09:44 PM, ext Nicolas Pitre wrote:
>  > On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:
>  >
>  >> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
>  >>> 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.
>  >
>  > 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.

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.

-- 
Tixy

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Viktor Rosendahl]

On 03/30/2011 06:52 PM, ext Tixy wrote:

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

best regards,

Viktor

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Tixy]

On Wed, 2011-03-30 at 19:46 +0300, Viktor Rosendahl wrote:
> On 03/30/2011 06:52 PM, ext Tixy wrote:
> 
> > 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.

I came to my conclusion because I was trying to verify the PC writeback
fix by looking at the ARM ARM and checking that all of the 20 or so
encodings [1] of LDR/STR instructions handled by the routine actually
had the prefix and writeback bits we were testing. I think they did, but
it was very tedious, and I thought I could easily miss something and
then we might end up introducing a new bug.

If ARM were still a RISC processor then things would be a lot easier ;-)

-- 
Tixy

[1] In ARM ARM, See
    Table A5-15 Single data transfer instructions
    Table A5-10 Extra load/store instructions
    Table A5-11 Extra load/store instructions (unprivileged)

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Nicolas Pitre]

On Wed, 30 Mar 2011, Tixy wrote:

> On Wed, 2011-03-30 at 16:42 +0300, Viktor Rosendahl wrote:
> > On 03/29/2011 09:44 PM, ext Nicolas Pitre wrote:
> >  > On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:
> >  >
> >  >> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
> >  >>> 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.
> >  >
> >  > 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.
> 
> 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.

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.


Nicolas

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Tixy]

On Wed, 2011-03-30 at 13:59 -0400, Nicolas Pitre wrote:
> On Wed, 30 Mar 2011, Tixy wrote:
> 
> > On Wed, 2011-03-30 at 16:42 +0300, Viktor Rosendahl wrote:
> > > On 03/29/2011 09:44 PM, ext Nicolas Pitre wrote:
> > >  > On Tue, 29 Mar 2011, Russell King - ARM Linux wrote:
> > >  >
> > >  >> On Tue, Mar 29, 2011 at 12:55:27PM -0400, Nicolas Pitre wrote:
> > >  >>> 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.
> > >  >
> > >  > 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.
> > 
> > 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.
> 
> 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.

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 ;-)

I'll get on with writing test code so that we can find bugs when probing
legal instructions...

-- 
Tixy

[PATCH] Reject kprobes when Rn==15 and writeback is set

[Nicolas Pitre]

On Wed, 30 Mar 2011, Tixy wrote:

> On Wed, 2011-03-30 at 13:59 -0400, Nicolas Pitre wrote:
> > 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.

Sure.  If the code was there in the first place then that's not our 
problem (kprobes hat on).  But we should never provide an opportunity 
for making it even less secure through emulation discrepancies that can 
be exploited.

> 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 ;-)

Right.  I think this rule should be carefully implemented.  We probably 
are OK being lax with other types of undefined behaviors.


Nicolas

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Viktor Rosendahl]

On 03/29/2011 07:55 PM, ext Nicolas Pitre wrote:
>
> Sorry, I meant r15-indexed with a write back.
>

OK, I guess it was kind of implicit in your message but I missed it.

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

I guess you are right, I missed that LDRD/STRD needs to be double word 
aligned for the older alignment models. In the ARMv7 model, word 
alignment is enough.

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

Yes, of course it's the literals. I was silly to think otherwise :)

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

Currently, we are not checking for that case at all, so the only missing 
part would be to modify the decoding logic. I just sent a patch for that.

best regards,

Viktor

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Tixy]

On Mon, 2011-03-28 at 18:56 +0300, Viktor Rosendahl wrote:
> On 03/25/2011 11:19 PM, ext Tixy wrote:
> > On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote:
> >> 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.

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

> 
> Signed-off-by: Viktor Rosendahl <viktor.rosendahl@nokia.com>
> ---
>  arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
>  1 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
> index 2389131..3b0cf90 100644
> --- a/arch/arm/kernel/kprobes-decode.c
> +++ b/arch/arm/kernel/kprobes-decode.c
> @@ -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. */
>  }


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;


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

-- 
Tixy

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Viktor Rosendahl]

On 03/29/2011 03:55 PM, ext Tixy wrote:

> 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 :)

Like I have said before, my suspicion is that most of the kprobe 
slowdown will come from the handling of illegal instruction exceptions 
when the probe is hit and at the end of the single stepping, so I don't 
think 3 loads and 3 stores in the emulation will do that much difference.

My suggestion is for you to submit a separate patch that sits on top of 
my patch 6853/1, if you want to change this code.

best regards,

Viktor

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Tixy]

On Tue, 2011-03-29 at 16:46 +0300, Viktor Rosendahl wrote:

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

Yes, its a minor performance and size issue only. When reviewing the
changes I saw that they were adding a few memory accesses which could
have be avoided.

> 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 :)

Fair enough :-)

-- 
Tixy

[PATCH] Fix ldrd/strd emulation for kprobes/ARM

[Nicolas Pitre]

On Tue, 29 Mar 2011, Tixy wrote:

> On Mon, 2011-03-28 at 18:56 +0300, Viktor Rosendahl wrote:
> > On 03/25/2011 11:19 PM, ext Tixy wrote:
> > > On Fri, 2011-03-25 at 19:01 +0200, Viktor Rosendahl wrote:
> > >> 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.
> 
> 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...
> 
> > 
> > Signed-off-by: Viktor Rosendahl <viktor.rosendahl@nokia.com>
> > ---
> >  arch/arm/kernel/kprobes-decode.c |   20 +++++++++++++++-----
> >  1 files changed, 15 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
> > index 2389131..3b0cf90 100644
> > --- a/arch/arm/kernel/kprobes-decode.c
> > +++ b/arch/arm/kernel/kprobes-decode.c
> > @@ -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. */
> >  }
> 
> 
> 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"
>  	);

I agree this is a nice improvement.  However it is best to keep fixes 
and improvements into separate patches.


Nicolas

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

[Viktor Rosendahl]

On 03/25/2011 11:19 PM, ext Tixy wrote:
>
>> 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 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".

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

Nice.

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

[Tixy]

On Mon, 2011-03-28 at 19:27 +0300, Viktor Rosendahl wrote:
> On 03/25/2011 11:19 PM, ext Tixy wrote:
[...]
> > 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".
[...]

Yes, I think you are correct. I jumped to conclusions from just looking
at the comments and not looking at the code carefully enough.

-- 
Tixy

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

[Nicolas Pitre]

On Fri, 25 Mar 2011, Viktor Rosendahl wrote:

> 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 <viktor.rosendahl@nokia.com>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>

Please send to RMK's patch system.

> ---
>  arch/arm/kernel/kprobes-decode.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/kernel/kprobes-decode.c b/arch/arm/kernel/kprobes-decode.c
> index 8f6ed43..2389131 100644
> --- a/arch/arm/kernel/kprobes-decode.c
> +++ b/arch/arm/kernel/kprobes-decode.c
> @@ -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)
> -- 
> 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
>