riscv: kprobe: Fixup misaligned load text
Subsystems (2)
PATCH 1/1 riscv: kprobe: Fixup misaligned load text
From: Guo Ren <[email protected]>
The current kprobe would cause a misaligned load for the probe point. This patch fixup it with two half-word loads instead.
--- arch/riscv/kernel/probes/kprobes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
arch/riscv/kernel/probes/kprobes.c +3 -1
@@ -74,7 +74,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return -EILSEQ ;/* copy instruction */ - p->opcode = *p->addr; + p->opcode = (kprobe_opcode_t)(*(u16 *)probe_addr); +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= (kprobe_opcode_t)(*(u16 *)(probe_addr +2 )) <<16 ;/* decode instruction */ switch (riscv_probe_decode_insn (p->addr, &p->ainsn.api)) {
Signature · 1 line
2.36.1
Quoted 19 lines · [email protected] writes:
From: Guo Ren <[email protected]>
The current kprobe would cause a misaligned load for the probe point. This patch fixup it with two half-word loads instead.
Fixes c22b0bcb1dd0 ("riscv: Add kprobes supported")Signed-off-by Guo Ren <[email protected]>Signed-off-by Guo Ren <[email protected]>Reported-by Bjorn Topel <[email protected]>--- arch/riscv/kernel/probes/kprobes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/riscv/kernel/probes/kprobes.c +0
-0
arch/riscv/kernel/probes/kprobes.c int __kprobes arch_prepare_kprobe(struct kprobe *p) · line 80
Quoted 8 lines
arch/riscv/kernel/probes/kprobes.c +3
-1@@ -74,7 +74,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return -EILSEQ ;/* copy instruction */ - p->opcode = *p->addr; + p->opcode = (kprobe_opcode_t)(*(u16 *)probe_addr); +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= (kprobe_opcode_t)(*(u16 *)(probe_addr +2 )) <<16 ;
Ugh, those casts. :-( What about the memcpy variant you had in the other thread?
Quoted 24 lines · Björn Töpel
Quoted 21 lines · [email protected] writes:
From: Guo Ren <[email protected]>
The current kprobe would cause a misaligned load for the probe point. This patch fixup it with two half-word loads instead.
Fixes c22b0bcb1dd0 ("riscv: Add kprobes supported")Signed-off-by Guo Ren <[email protected]>Signed-off-by Guo Ren <[email protected]>Reported-by Bjorn Topel <[email protected]>--- arch/riscv/kernel/probes/kprobes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/riscv/kernel/probes/kprobes.c +0
-0@@ -74,7 +74,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return -EILSEQ ;
Quoted 9 lines
arch/riscv/kernel/probes/kprobes.c int __kprobes arch_prepare_kprobe(struct kprobe *p)
Quoted 6 lines
arch/riscv/kernel/probes/kprobes.c +3
-1/* copy instruction */ - p->opcode = *p->addr; + p->opcode = (kprobe_opcode_t)(*(u16 *)probe_addr); +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= (kprobe_opcode_t)(*(u16 *)(probe_addr +2 )) <<16 ;Ugh, those casts. :-( What about the memcpy variant you had in the other thread?
The memcpy version would force load probe_addr + 2. This one would save an lh operation. The code text guarantees half-word alignment. No misaligned load happened. Second, kprobe wouldn't write the last half of 32b instruction.
Signature · 2 lines
Best Regards Guo Ren
Quoted 34 lines · Guo Ren
Quoted 24 lines · Björn Töpel
Quoted 21 lines · [email protected] writes:
From: Guo Ren <[email protected]>
The current kprobe would cause a misaligned load for the probe point. This patch fixup it with two half-word loads instead.
Fixes c22b0bcb1dd0 ("riscv: Add kprobes supported")Signed-off-by Guo Ren <[email protected]>Signed-off-by Guo Ren <[email protected]>Reported-by Bjorn Topel <[email protected]>--- arch/riscv/kernel/probes/kprobes.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)arch/riscv/kernel/probes/kprobes.c +0
-0@@ -74,7 +74,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return -EILSEQ ;Quoted 9 lines
arch/riscv/kernel/probes/kprobes.c int __kprobes arch_prepare_kprobe(struct kprobe *p) · line 80
Quoted 6 lines
arch/riscv/kernel/probes/kprobes.c +3
-1/* copy instruction */ - p->opcode = *p->addr; + p->opcode = (kprobe_opcode_t)(*(u16 *)probe_addr); +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= (kprobe_opcode_t)(*(u16 *)(probe_addr +2 )) <<16 ;Ugh, those casts. :-( What about the memcpy variant you had in the other thread?
Quoted 4 lines
The memcpy version would force load
probe_addr+ 2. This one would save an lh operation. The code text guarantees half-word alignment. No misaligned load happened. Second, kprobe wouldn't write the last half of 32b instruction.
Ok, something more readable, like:
arch/riscv/kernel/probes/kprobes.c +6 -4
@@ -50,14 +50,16 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)int __kprobes arch_prepare_kprobe (struct kprobe *p) { -unsigned long probe_addr = (unsigned long )p->addr; +u16 *insn = (u16 *)p->addr; -if (probe_addr &0x1 ) +if ((uintptr_t )insn &0x1 )return -EILSEQ ;/* copy instruction */ - p->opcode = *p->addr; - + p->opcode = *insn++; +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= *insn <<16 ; +/* decode instruction */ switch (riscv_probe_decode_insn (p->addr, &p->ainsn.api)) {case INSN_REJECTED :/* insn not supported */
Björn
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Quoted 55 lines · Björn Töpel
Quoted 34 lines · Guo Ren
Quoted 24 lines · Björn Töpel
Quoted 21 lines · [email protected] writes:
From: Guo Ren <[email protected]>
The current kprobe would cause a misaligned load for the probe point. This patch fixup it with two half-word loads instead.
Fixes c22b0bcb1dd0 ("riscv: Add kprobes supported")Signed-off-by Guo Ren <[email protected]>Signed-off-by Guo Ren <[email protected]>Reported-by Bjorn Topel <[email protected]>---
arch/riscv/kernel/probes/kprobes.c| 4 +++-1 file changed, 3 insertions(+), 1 deletion(-)arch/riscv/kernel/probes/kprobes.c +0
-0@@ -74,7 +74,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)return -EILSEQ ;Quoted 9 lines
arch/riscv/kernel/probes/kprobes.c static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
Quoted 6 lines
arch/riscv/kernel/probes/kprobes.c +3
-1/* copy instruction */ - p->opcode = *p->addr; + p->opcode = (kprobe_opcode_t)(*(u16 *)probe_addr); +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= (kprobe_opcode_t)(*(u16 *)(probe_addr +2 )) <<16 ;Ugh, those casts. :-( What about the memcpy variant you had in the other thread?
Quoted 4 lines
The memcpy version would force load
probe_addr+ 2. This one would save an lh operation. The code text guarantees half-word alignment. No misaligned load happened. Second, kprobe wouldn't write the last half of 32b instruction.Ok, something more readable, like:
arch/riscv/kernel/probes/kprobes.c +1
-1@@ -50,14 +50,16 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)int __kprobes arch_prepare_kprobe (struct kprobe *p) { -unsigned long probe_addr = (unsigned long )p->addr; +u16 *insn = (u16 *)p->addr;
arch/riscv/kernel/probes/kprobes.c static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) · line 61
Quoted 10 lines
arch/riscv/kernel/probes/kprobes.c +4
-3-if (probe_addr &0x1 ) +if ((uintptr_t )insn &0x1 )return -EILSEQ ;/* copy instruction */ - p->opcode = *p->addr; - + p->opcode = *insn++; +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= *insn <<16 ;
*insn gets promoted to int not unsigned so this is UB if bit 15 is set.
Jess
Quoted 12 lines
arch/riscv/kernel/probes/kprobes.c +1
-0+/* decode instruction */ switch (riscv_probe_decode_insn (p->addr, &p->ainsn.api)) {case INSN_REJECTED :/* insn not supported */ Björn
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Quoted 5 lines · Jessica Clarke
arch/riscv/kernel/probes/kprobes.c static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) · line 61
Quoted 3 lines
arch/riscv/kernel/probes/kprobes.c +3
-0+ p->opcode = *insn++; +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= *insn <<16 ;*insn gets promoted to int not unsigned so this is UB if bit 15 is set.
Ugh. Good catch! I guess we can't get rid of *that* explicit cast to kprobe_opcode_t here...
Quoted 11 lines · Björn Töpel
Quoted 5 lines · Jessica Clarke
arch/riscv/kernel/probes/kprobes.c static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) · line 61
Quoted 3 lines
arch/riscv/kernel/probes/kprobes.c +3
-0+ p->opcode = *insn++; +if (GET_INSN_LENGTH (p->opcode) ==4 ) + p->opcode |= *insn <<16 ;*insn gets promoted to int not unsigned so this is UB if bit 15 is set.
Ugh. Good catch! I guess we can't get rid of *that* explicit cast to
kprobe_opcode_there...
Hi Bjorn & Jessica,
Thx for reviewing.
The new version came out:
https://lore.kernel.org/linux-riscv/[email protected]/
--
Best Regards
Guo Ren