[PATCH] riscv: kprobes: decode instruction from copied opcode

[PATCH] riscv: kprobes: decode instruction from copied opcode

[Xiaofeng Yuan]

riscv_probe_decode_insn() reads the probed instruction with a raw
32-bit load:

	probe_opcode_t insn = *addr;

If the instruction is compressed and sits in the last two bytes of a
module text page, this load crosses into the following vmalloc guard
page and faults.  arch_prepare_kprobe() already copies the instruction
in two 16-bit steps into p->opcode, so pass that value by value instead
of reading the probed address again.

Do the same in arch_uprobe_analyze_insn() and arch_uprobe_skip_sstep(),
which still read auprobe->insn[] with a raw 32-bit load, for
consistency with arch_prepare_kprobe().

Link: https://lore.kernel.org/all/undine-1792411157938575056-598130d3332901bd@undined.local
Signed-off-by: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
---
 arch/riscv/kernel/probes/decode-insn.c |  4 +---
 arch/riscv/kernel/probes/decode-insn.h |  2 +-
 arch/riscv/kernel/probes/kprobes.c     |  2 +-
 arch/riscv/kernel/probes/uprobes.c     | 11 ++++++++---
 4 files changed, 11 insertions(+), 8 deletions(-)

Tested on QEMU riscv64: the in-tree kprobes KUnit tests pass, and a
userspace uprobe smoke test (compressed c.j and 32-bit jal probe sites)
exercises arch_uprobe_analyze_insn() and arch_uprobe_skip_sstep() with
the new two-step 16-bit reads.  The cross-page fault cannot be
reproduced in QEMU (it needs a module text page of size % 4096 == 4094),
so it is argued statically and confirmed by disassembly that the raw
load is gone.

diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
index 433d9035b..4a298de88 100644
--- a/arch/riscv/kernel/probes/decode-insn.c
+++ b/arch/riscv/kernel/probes/decode-insn.c
@@ -14,10 +14,8 @@
  *   INSN_GOOD_NO_SLOT If instruction is supported but doesn't use its slot.
  */
 enum probe_insn __kprobes
-riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)
+riscv_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *api)
 {
-	probe_opcode_t insn = *addr;
-
 	/*
 	 * Reject instructions list:
 	 */
diff --git a/arch/riscv/kernel/probes/decode-insn.h b/arch/riscv/kernel/probes/decode-insn.h
index 42269a7d6..0064bc76e 100644
--- a/arch/riscv/kernel/probes/decode-insn.h
+++ b/arch/riscv/kernel/probes/decode-insn.h
@@ -13,6 +13,6 @@ enum probe_insn {
 };
 
 enum probe_insn __kprobes
-riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *asi);
+riscv_probe_decode_insn(probe_opcode_t insn, struct arch_probe_insn *asi);
 
 #endif /* _RISCV_KERNEL_KPROBES_DECODE_INSN_H */
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 9e2afabf9..d9274b530 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -85,7 +85,7 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 		p->opcode |= (kprobe_opcode_t)(*insn) << 16;
 
 	/* decode instruction */
-	switch (riscv_probe_decode_insn(p->addr, &p->ainsn.api)) {
+	switch (riscv_probe_decode_insn(p->opcode, &p->ainsn.api)) {
 	case INSN_REJECTED:	/* insn not supported */
 		return -EINVAL;
 
diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c
index eb177d0ce..8c003b7de 100644
--- a/arch/riscv/kernel/probes/uprobes.c
+++ b/arch/riscv/kernel/probes/uprobes.c
@@ -32,12 +32,15 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,
 			     unsigned long addr)
 {
 	probe_opcode_t opcode;
+	u16 *insn = (u16 *)&auprobe->insn[0];
 
-	opcode = *(probe_opcode_t *)(&auprobe->insn[0]);
+	opcode = (probe_opcode_t)(*insn++);
+	if (GET_INSN_LENGTH(opcode) == 4)
+		opcode |= (probe_opcode_t)(*insn) << 16;
 
 	auprobe->insn_size = GET_INSN_LENGTH(opcode);
 
-	switch (riscv_probe_decode_insn(&opcode, &auprobe->api)) {
+	switch (riscv_probe_decode_insn(opcode, &auprobe->api)) {
 	case INSN_REJECTED:
 		return -EINVAL;
 
@@ -96,7 +99,9 @@ bool arch_uprobe_skip_sstep(struct arch_uprobe *auprobe, struct pt_regs *regs)
 	if (!auprobe->simulate)
 		return false;
 
-	insn = *(probe_opcode_t *)(&auprobe->insn[0]);
+	insn = (probe_opcode_t)(*(u16 *)&auprobe->insn[0]);
+	if (auprobe->insn_size == 4)
+		insn |= (probe_opcode_t)(*(u16 *)&auprobe->insn[2]) << 16;
 	addr = instruction_pointer(regs);
 
 	if (auprobe->api.handler)
-- 
2.34.1