[PATCH 1/4] ARM: kprobes: fix instruction fetch order with <asm/opcodes.h>
Ben Dooks
ben.dooks at codethink.co.uk
Fri Aug 2 09:58:57 EDT 2013
On 02/08/13 14:57, Dave Martin wrote:
> On Wed, Jul 31, 2013 at 08:43:07PM +0100, Ben Dooks wrote:
>> On 29/07/13 09:01, Jon Medhurst (Tixy) wrote:
>>> On Thu, 2013-07-25 at 22:08 +0100, Ben Dooks wrote:
>>>> If we are running BE8, the data and instruction endian-ness do not
>>>> match, so use<asm/opcodes.h> to correctly translate memory accesses
>>>> into ARM instructions.
>>>>
>>>> Signed-off-by: Ben Dooks<ben.dooks at codethink.co.uk>y
>>>> ---
>>>> arch/arm/kernel/kprobes-common.c | 14 ++++++++------
>>>> arch/arm/kernel/kprobes.c | 9 +++++----
>>>> 2 files changed, 13 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/arch/arm/kernel/kprobes-common.c b/arch/arm/kernel/kprobes-common.c
>>>> index 18a7628..c0b202e 100644
>>>> --- a/arch/arm/kernel/kprobes-common.c
>>>> +++ b/arch/arm/kernel/kprobes-common.c
>>>> @@ -14,6 +14,7 @@
>>>> #include<linux/kernel.h>
>>>> #include<linux/kprobes.h>
>>>> #include<asm/system_info.h>
>>>> +#include<asm/opcodes.h>
>>>>
>>>> #include "kprobes.h"
>>>>
>>>> @@ -305,7 +306,8 @@ kprobe_decode_ldmstm(kprobe_opcode_t insn, struct arch_specific_insn *asi)
>>>>
>>>> if (handler) {
>>>> /* We can emulate the instruction in (possibly) modified form */
>>>> - asi->insn[0] = (insn& 0xfff00000) | (rn<< 16) | reglist;
>>>> + asi->insn[0] = __opcode_to_mem_arm((insn& 0xfff00000) |
>>>> + (rn<< 16) | reglist);
>>>> asi->insn_handler = handler;
>>>> return INSN_GOOD;
>>>> }
>>>> @@ -338,9 +340,9 @@ prepare_emulated_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
>>>> thumb_insn[2] = 0x4770; /* Thumb bx lr */
>>>
>>> The line above and the one before it not in this diff also need fixing
>>> by changing to __opcode_to_mem_thumb16(0x4770).
>>
>> IIRC, these are then fixed by the set_emulated instruction, which is
>> also why the insn return is not swapped.
>>
>>>> return insn;
>>>> }
>>>> - asi->insn[1] = 0xe12fff1e; /* ARM bx lr */
>>>> + asi->insn[1] = __opcode_to_mem_arm(0xe12fff1e); /* ARM bx lr */
>>>> #else
>>>> - asi->insn[1] = 0xe1a0f00e; /* mov pc, lr */
>>>> + asi->insn[1] = __opcode_to_mem_arm(0xe1a0f00e); /* mov pc, lr */
>>>> #endif
>>>> /* Make an ARM instruction unconditional */
>>>> if (insn< 0xe0000000)
>>>> @@ -360,12 +362,12 @@ set_emulated_insn(kprobe_opcode_t insn, struct arch_specific_insn *asi,
>>>> if (thumb) {
>>>> u16 *ip = (u16 *)asi->insn;
>>>> if (is_wide_instruction(insn))
>>>> - *ip++ = insn>> 16;
>>>> - *ip++ = insn;
>>>> + *ip++ = ___asm_opcode_to_mem_thumb16(insn>> 16);
>>>> + *ip++ = ___asm_opcode_to_mem_thumb16(insn);
>>>
>>>
>>> Should these two ___asm_opcode_thumb32_compose() not be
>>> __opcode_thumb32_compose() ?
>>
>> I decided to leave this as is.
>
> Although the intent is not made as clear in the header as it could be,
> the ___asm() functions are not supposed to be used directly:
> __opcode_thumb32_compose() will map to the correct thing based on
> whether we are preprocessing assembler or C.
>
> In particular, ___asm_opcode_thumb32_compose() will give you inefficient
> swabbing if used directly in C.
>
>>>> return;
>>>> }
>>>> #endif
>>>> - asi->insn[0] = insn;
>>>> + asi->insn[0] = __opcode_to_mem_arm(insn);
>>>> }
>>>>
>>>> /*
>>>> diff --git a/arch/arm/kernel/kprobes.c b/arch/arm/kernel/kprobes.c
>>>> index 170e9f3..3775af1 100644
>>>> --- a/arch/arm/kernel/kprobes.c
>>>> +++ b/arch/arm/kernel/kprobes.c
>>>> @@ -26,6 +26,7 @@
>>>> #include<linux/stop_machine.h>
>>>> #include<linux/stringify.h>
>>>> #include<asm/traps.h>
>>>> +#include<asm/opcodes.h>
>>>> #include<asm/cacheflush.h>
>>>>
>>>> #include "kprobes.h"
>>>> @@ -62,10 +63,10 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>>>> #ifdef CONFIG_THUMB2_KERNEL
>>>> thumb = true;
>>>> addr&= ~1; /* Bit 0 would normally be set to indicate Thumb code */
>>>> - insn = ((u16 *)addr)[0];
>>>> + insn = __mem_to_opcode_thumb16(((u16 *)addr)[0]);
>>>> if (is_wide_instruction(insn)) {
>>>> - insn<<= 16;
>>>> - insn |= ((u16 *)addr)[1];
>>>> + u16 inst2 = __mem_to_opcode_thumb16(((u16 *)addr)[1]);
>>>> + insn = ___asm_opcode_thumb32_compose(insn, inst2);
>>>
>>> Again, __opcode_thumb32_compose instead of
>>> ___asm_opcode_thumb32_compose
>
> ditto, this should be changed.
>
> Cheers
> ---Dave
Yes, already fixed thanks.
--
Ben Dooks http://www.codethink.co.uk/
Senior Engineer Codethink - Providing Genius
More information about the linux-arm-kernel
mailing list