* [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC @ 2011-03-25 17:01 Viktor Rosendahl 2011-03-25 21:19 ` Tixy 2011-03-26 2:03 ` Nicolas Pitre 0 siblings, 2 replies; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-25 17:01 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC 2011-03-25 17:01 [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl @ 2011-03-25 21:19 ` Tixy 2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 2011-03-28 16:27 ` [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl 2011-03-26 2:03 ` Nicolas Pitre 1 sibling, 2 replies; 23+ messages in thread From: Tixy @ 2011-03-25 21:19 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-25 21:19 ` Tixy @ 2011-03-28 15:56 ` Viktor Rosendahl 2011-03-28 22:39 ` Nicolas Pitre 2011-03-29 12:55 ` Tixy 2011-03-28 16:27 ` [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl 1 sibling, 2 replies; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-28 15:56 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl @ 2011-03-28 22:39 ` Nicolas Pitre 2011-03-29 11:26 ` Viktor Rosendahl 2011-03-29 12:55 ` Tixy 1 sibling, 1 reply; 23+ messages in thread From: Nicolas Pitre @ 2011-03-28 22:39 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-28 22:39 ` Nicolas Pitre @ 2011-03-29 11:26 ` Viktor Rosendahl 2011-03-29 16:55 ` Nicolas Pitre 0 siblings, 1 reply; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-29 11:26 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 11:26 ` Viktor Rosendahl @ 2011-03-29 16:55 ` Nicolas Pitre 2011-03-29 18:31 ` Russell King - ARM Linux 2011-03-30 14:09 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 0 siblings, 2 replies; 23+ messages in thread From: Nicolas Pitre @ 2011-03-29 16:55 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 16:55 ` Nicolas Pitre @ 2011-03-29 18:31 ` Russell King - ARM Linux 2011-03-29 18:44 ` Nicolas Pitre 2011-03-30 14:09 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 1 sibling, 1 reply; 23+ messages in thread From: Russell King - ARM Linux @ 2011-03-29 18:31 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 18:31 ` Russell King - ARM Linux @ 2011-03-29 18:44 ` Nicolas Pitre 2011-03-30 13:42 ` [PATCH] Reject kprobes when Rn==15 and writeback is set Viktor Rosendahl 0 siblings, 1 reply; 23+ messages in thread From: Nicolas Pitre @ 2011-03-29 18:44 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-29 18:44 ` Nicolas Pitre @ 2011-03-30 13:42 ` Viktor Rosendahl 2011-03-30 15:52 ` Tixy 0 siblings, 1 reply; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-30 13:42 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 13:42 ` [PATCH] Reject kprobes when Rn==15 and writeback is set Viktor Rosendahl @ 2011-03-30 15:52 ` Tixy 2011-03-30 16:46 ` Viktor Rosendahl 2011-03-30 17:59 ` Nicolas Pitre 0 siblings, 2 replies; 23+ messages in thread From: Tixy @ 2011-03-30 15:52 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 15:52 ` Tixy @ 2011-03-30 16:46 ` Viktor Rosendahl 2011-03-30 17:20 ` Tixy 2011-03-30 17:59 ` Nicolas Pitre 1 sibling, 1 reply; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-30 16:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 16:46 ` Viktor Rosendahl @ 2011-03-30 17:20 ` Tixy 0 siblings, 0 replies; 23+ messages in thread From: Tixy @ 2011-03-30 17:20 UTC (permalink / raw) To: linux-arm-kernel 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) ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 15:52 ` Tixy 2011-03-30 16:46 ` Viktor Rosendahl @ 2011-03-30 17:59 ` Nicolas Pitre 2011-03-30 19:39 ` Tixy 1 sibling, 1 reply; 23+ messages in thread From: Nicolas Pitre @ 2011-03-30 17:59 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 17:59 ` Nicolas Pitre @ 2011-03-30 19:39 ` Tixy 2011-03-30 20:48 ` Nicolas Pitre 0 siblings, 1 reply; 23+ messages in thread From: Tixy @ 2011-03-30 19:39 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Reject kprobes when Rn==15 and writeback is set 2011-03-30 19:39 ` Tixy @ 2011-03-30 20:48 ` Nicolas Pitre 0 siblings, 0 replies; 23+ messages in thread From: Nicolas Pitre @ 2011-03-30 20:48 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 16:55 ` Nicolas Pitre 2011-03-29 18:31 ` Russell King - ARM Linux @ 2011-03-30 14:09 ` Viktor Rosendahl 1 sibling, 0 replies; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-30 14:09 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 2011-03-28 22:39 ` Nicolas Pitre @ 2011-03-29 12:55 ` Tixy 2011-03-29 13:46 ` Viktor Rosendahl 2011-03-29 17:07 ` Nicolas Pitre 1 sibling, 2 replies; 23+ messages in thread From: Tixy @ 2011-03-29 12:55 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 12:55 ` Tixy @ 2011-03-29 13:46 ` Viktor Rosendahl 2011-03-29 14:03 ` Tixy 2011-03-29 17:07 ` Nicolas Pitre 1 sibling, 1 reply; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-29 13:46 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 13:46 ` Viktor Rosendahl @ 2011-03-29 14:03 ` Tixy 0 siblings, 0 replies; 23+ messages in thread From: Tixy @ 2011-03-29 14:03 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] Fix ldrd/strd emulation for kprobes/ARM 2011-03-29 12:55 ` Tixy 2011-03-29 13:46 ` Viktor Rosendahl @ 2011-03-29 17:07 ` Nicolas Pitre 1 sibling, 0 replies; 23+ messages in thread From: Nicolas Pitre @ 2011-03-29 17:07 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC 2011-03-25 21:19 ` Tixy 2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl @ 2011-03-28 16:27 ` Viktor Rosendahl 2011-03-29 9:12 ` Tixy 1 sibling, 1 reply; 23+ messages in thread From: Viktor Rosendahl @ 2011-03-28 16:27 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC 2011-03-28 16:27 ` [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl @ 2011-03-29 9:12 ` Tixy 0 siblings, 0 replies; 23+ messages in thread From: Tixy @ 2011-03-29 9:12 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC 2011-03-25 17:01 [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl 2011-03-25 21:19 ` Tixy @ 2011-03-26 2:03 ` Nicolas Pitre 1 sibling, 0 replies; 23+ messages in thread From: Nicolas Pitre @ 2011-03-26 2:03 UTC (permalink / raw) To: linux-arm-kernel 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 > ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2011-03-30 20:48 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-25 17:01 [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl 2011-03-25 21:19 ` Tixy 2011-03-28 15:56 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 2011-03-28 22:39 ` Nicolas Pitre 2011-03-29 11:26 ` Viktor Rosendahl 2011-03-29 16:55 ` Nicolas Pitre 2011-03-29 18:31 ` Russell King - ARM Linux 2011-03-29 18:44 ` Nicolas Pitre 2011-03-30 13:42 ` [PATCH] Reject kprobes when Rn==15 and writeback is set Viktor Rosendahl 2011-03-30 15:52 ` Tixy 2011-03-30 16:46 ` Viktor Rosendahl 2011-03-30 17:20 ` Tixy 2011-03-30 17:59 ` Nicolas Pitre 2011-03-30 19:39 ` Tixy 2011-03-30 20:48 ` Nicolas Pitre 2011-03-30 14:09 ` [PATCH] Fix ldrd/strd emulation for kprobes/ARM Viktor Rosendahl 2011-03-29 12:55 ` Tixy 2011-03-29 13:46 ` Viktor Rosendahl 2011-03-29 14:03 ` Tixy 2011-03-29 17:07 ` Nicolas Pitre 2011-03-28 16:27 ` [PATCH] kprobes/arm: fix emulation of LDR/STR instruction when Rn == PC Viktor Rosendahl 2011-03-29 9:12 ` Tixy 2011-03-26 2:03 ` Nicolas Pitre
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).