Skip to content

Commit 13f8f1e

Browse files
mrutland-armwilldeacon
authored andcommitted
arm64: probes: Fix uprobes for big-endian kernels
The arm64 uprobes code is broken for big-endian kernels as it doesn't convert the in-memory instruction encoding (which is always little-endian) into the kernel's native endianness before analyzing and simulating instructions. This may result in a few distinct problems: * The kernel may may erroneously reject probing an instruction which can safely be probed. * The kernel may erroneously erroneously permit stepping an instruction out-of-line when that instruction cannot be stepped out-of-line safely. * The kernel may erroneously simulate instruction incorrectly dur to interpretting the byte-swapped encoding. The endianness mismatch isn't caught by the compiler or sparse because: * The arch_uprobe::{insn,ixol} fields are encoded as arrays of u8, so the compiler and sparse have no idea these contain a little-endian 32-bit value. The core uprobes code populates these with a memcpy() which similarly does not handle endianness. * While the uprobe_opcode_t type is an alias for __le32, both arch_uprobe_analyze_insn() and arch_uprobe_skip_sstep() cast from u8[] to the similarly-named probe_opcode_t, which is an alias for u32. Hence there is no endianness conversion warning. Fix this by changing the arch_uprobe::{insn,ixol} fields to __le32 and adding the appropriate __le32_to_cpu() conversions prior to consuming the instruction encoding. The core uprobes copies these fields as opaque ranges of bytes, and so is unaffected by this change. At the same time, remove MAX_UINSN_BYTES and consistently use AARCH64_INSN_SIZE for clarity. Tested with the following: | #include <stdio.h> | #include <stdbool.h> | | #define noinline __attribute__((noinline)) | | static noinline void *adrp_self(void) | { | void *addr; | | asm volatile( | " adrp %x0, adrp_self\n" | " add %x0, %x0, :lo12:adrp_self\n" | : "=r" (addr)); | } | | | int main(int argc, char *argv) | { | void *ptr = adrp_self(); | bool equal = (ptr == adrp_self); | | printf("adrp_self => %p\n" | "adrp_self() => %p\n" | "%s\n", | adrp_self, ptr, equal ? "EQUAL" : "NOT EQUAL"); | | return 0; | } .... where the adrp_self() function was compiled to: | 00000000004007e0 <adrp_self>: | 4007e0: 90000000 adrp x0, 400000 <__ehdr_start> | 4007e4: 911f8000 add x0, x0, #0x7e0 | 4007e8: d65f03c0 ret Before this patch, the ADRP is not recognized, and is assumed to be steppable, resulting in corruption of the result: | # ./adrp-self | adrp_self => 0x4007e0 | adrp_self() => 0x4007e0 | EQUAL | # echo 'p /root/adrp-self:0x007e0' > /sys/kernel/tracing/uprobe_events | # echo 1 > /sys/kernel/tracing/events/uprobes/enable | # ./adrp-self | adrp_self => 0x4007e0 | adrp_self() => 0xffffffffff7e0 | NOT EQUAL After this patch, the ADRP is correctly recognized and simulated: | # ./adrp-self | adrp_self => 0x4007e0 | adrp_self() => 0x4007e0 | EQUAL | # | # echo 'p /root/adrp-self:0x007e0' > /sys/kernel/tracing/uprobe_events | # echo 1 > /sys/kernel/tracing/events/uprobes/enable | # ./adrp-self | adrp_self => 0x4007e0 | adrp_self() => 0x4007e0 | EQUAL Fixes: 9842cea ("arm64: Add uprobe support") Cc: [email protected] Signed-off-by: Mark Rutland <[email protected]> Cc: Catalin Marinas <[email protected]> Cc: Will Deacon <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Will Deacon <[email protected]>
1 parent 50f813e commit 13f8f1e

File tree

2 files changed

+5
-7
lines changed

2 files changed

+5
-7
lines changed

arch/arm64/include/asm/uprobes.h

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,9 @@
1010
#include <asm/insn.h>
1111
#include <asm/probes.h>
1212

13-
#define MAX_UINSN_BYTES AARCH64_INSN_SIZE
14-
1513
#define UPROBE_SWBP_INSN cpu_to_le32(BRK64_OPCODE_UPROBES)
1614
#define UPROBE_SWBP_INSN_SIZE AARCH64_INSN_SIZE
17-
#define UPROBE_XOL_SLOT_BYTES MAX_UINSN_BYTES
15+
#define UPROBE_XOL_SLOT_BYTES AARCH64_INSN_SIZE
1816

1917
typedef __le32 uprobe_opcode_t;
2018

@@ -23,8 +21,8 @@ struct arch_uprobe_task {
2321

2422
struct arch_uprobe {
2523
union {
26-
u8 insn[MAX_UINSN_BYTES];
27-
u8 ixol[MAX_UINSN_BYTES];
24+
__le32 insn;
25+
__le32 ixol;
2826
};
2927
struct arch_probe_insn api;
3028
bool simulate;

arch/arm64/kernel/probes/uprobes.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
4242
else if (!IS_ALIGNED(addr, AARCH64_INSN_SIZE))
4343
return -EINVAL;
4444

45-
insn = *(probe_opcode_t *)(&auprobe->insn[0]);
45+
insn = le32_to_cpu(auprobe->insn);
4646

4747
switch (arm_probe_decode_insn(insn, &auprobe->api)) {
4848
case INSN_REJECTED:
@@ -108,7 +108,7 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
108108
if (!auprobe->simulate)
109109
return false;
110110

111-
insn = *(probe_opcode_t *)(&auprobe->insn[0]);
111+
insn = le32_to_cpu(auprobe->insn);
112112
addr = instruction_pointer(regs);
113113

114114
if (auprobe->api.handler)

0 commit comments

Comments
 (0)