* [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] 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
* [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] 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] 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] 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] 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-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 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 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] 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] 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] 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
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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.