Re: [PATCH v4 0/2] riscv: kprobes: reject probes inside LR/SC sequences

Re: [PATCH v4 0/2] riscv: kprobes: reject probes inside LR/SC sequences

[Nam Cao]

Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com> writes:
> A breakpoint trap taken in the middle of an LR/SC sequence clears the
> load reservation, so an SC following the probed instruction always fails
> and the enclosing retry loop re-enters the breakpoint, livelocking the
> CPU.
>
> This series makes the RISC-V kprobes implementation reject probes placed
> inside an LR/SC sequence:

CC: kprobes maintainers, I would appreciate your opinion on this. Should
kprobes reject it, or leave it to users to take care not to do that?

Nam

[PATCH v5 0/2] riscv: kprobes: reject probes inside LR/SC sequences

[Xiaofeng Yuan]

A breakpoint trap taken in the middle of an LR/SC sequence clears the
load reservation, so an SC following the probed instruction always fails
and the enclosing retry loop re-enters the breakpoint, livelocking the
CPU.  This series makes the RISC-V kprobes implementation reject probes
placed on an LR/SC instruction or between them, following arm64's
approach for its equivalent exclusive load/store instructions.

Changes since v4:
- Reject the LR and SC instructions themselves in the decode stage
  (riscv_probe_decode_insn()), so a probe placed directly on them is
  caught independently of the kallsyms offset (fixes the case where the
  probe address is exactly the enclosing symbol's start).
- Bound the sequence scan by instruction count (MAX_LRSC_GAP = 16)
  instead of bytes, matching the Zalrsc "at most 16 instructions"
  constraint and avoiding ambiguity with compressed instructions.
- Fold the LR/SC scan into arch_check_kprobe() so the function is walked
  only once; arch_check_kprobe() now returns an error code.
- Use the existing GET_INSN_LENGTH() macro and a single unaligned 32-bit
  load instead of two separate loads.
- Extend the KUnit test to check a probe right before, on the LR, inside,
  on the SC, and right after the sequence, storing absolute probe
  addresses instead of offsets.

Verified by cross-compiling for RISC-V and running the kprobes KUnit
test in QEMU (ok 2 kprobes_riscv).

Xiaofeng Yuan (2):
  riscv: kprobes: reject probes inside LR/SC sequences
  riscv: kprobes: add KUnit test for LR/SC sequence rejection

 arch/riscv/include/asm/insn.h                 | 33 ++++++++++
 arch/riscv/kernel/probes/decode-insn.c        |  2 +
 arch/riscv/kernel/probes/kprobes.c            | 63 +++++++++++++++----
 .../kernel/tests/kprobes/test-kprobes-asm.S   | 24 +++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.c | 26 ++++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.h |  3 +
 6 files changed, 139 insertions(+), 12 deletions(-)

-- 
2.34.1

[PATCH v5 1/2] riscv: kprobes: reject probes inside LR/SC sequences

[Xiaofeng Yuan]

A trap taken in the middle of an LR/SC sequence clears the load
reservation, so an SC following the probed instruction always fails and
the enclosing retry loop re-enters the breakpoint, livelocking the CPU.
Reject probes of the LR/SC instructions themselves and of any address
between them.

arm64's kprobes rejects its equivalent exclusive load/store instructions
and, in arm_kprobe_decode_insn(), walks backward from the probe address
(bounded by the enclosing symbol) to reject any address inside an
exclusive sequence.  RISC-V cannot walk backward because
variable-length instructions make instruction boundaries ambiguous, so
walk forward from the enclosing symbol, a known instruction boundary, up
to the probe address and track whether an LR is still outstanding.

A Zalrsc-constrained LR/SC retry loop holds at most 16 instructions, so
an LR more than MAX_LRSC_GAP instructions ahead of the probe cannot
start a sequence containing it.  Count instructions, not bytes:
compressed instructions would make a byte window ambiguous.

The LR/SC instructions themselves are rejected in
riscv_probe_decode_insn(), and an address inside a sequence is rejected
in arch_check_kprobe().

Signed-off-by: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
---
 arch/riscv/include/asm/insn.h          | 33 ++++++++++++++
 arch/riscv/kernel/probes/decode-insn.c |  2 +
 arch/riscv/kernel/probes/kprobes.c     | 63 +++++++++++++++++++++-----
 3 files changed, 86 insertions(+), 12 deletions(-)

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index ad348d8b8..46a1666d2 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -141,6 +141,7 @@
 #define RVG_OPCODE_JALR		0x67
 #define RVG_OPCODE_JAL		0x6f
 #define RVG_OPCODE_SYSTEM	0x73
+#define RVG_OPCODE_AMO		0x2f
 #define RVG_SYSTEM_CSR_OFF	20
 #define RVG_SYSTEM_CSR_MASK	GENMASK(11, 0)
 
@@ -175,6 +176,8 @@
 #define RVG_FUNCT3_BGE		0x5
 #define RVG_FUNCT3_BLTU		0x6
 #define RVG_FUNCT3_BGEU		0x7
+#define RVG_FUNCT3_AMO_W	0x2
+#define RVG_FUNCT3_AMO_D	0x3
 
 /* parts of funct3 code for C extension*/
 #define RVC_FUNCT3_C_BEQZ	0x6
@@ -263,6 +266,36 @@ __RISCV_INSN_FUNCS(ebreak, RVG_MASK_EBREAK, RVG_MATCH_EBREAK)
 __RISCV_INSN_FUNCS(sret, RVG_MASK_SRET, RVG_MATCH_SRET)
 __RISCV_INSN_FUNCS(fence, RVG_MASK_FENCE, RVG_MATCH_FENCE);
 
+/*
+ * LR/SC are identified by the AMO opcode plus the funct3 width field
+ * (.w/.d) and the funct5 field (funct7[6:2]) distinguishing LR from SC.
+ * The aq/rl bits (funct7[1:0]) are ignored.  For LR the rs2 field must
+ * encode x0.
+ */
+#define RVG_AMO_FUNCT5_LR	0x02
+#define RVG_AMO_FUNCT5_SC	0x03
+
+static __always_inline bool riscv_insn_is_lr(u32 code)
+{
+	u32 funct5 = (code >> 27) & 0x1f;
+	u32 funct3 = (code >> 12) & 0x7;
+
+	return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_AMO &&
+	       funct5 == RVG_AMO_FUNCT5_LR &&
+	       (funct3 == RVG_FUNCT3_AMO_W || funct3 == RVG_FUNCT3_AMO_D) &&
+	       !(code & GENMASK(24, 20));
+}
+
+static __always_inline bool riscv_insn_is_sc(u32 code)
+{
+	u32 funct5 = (code >> 27) & 0x1f;
+	u32 funct3 = (code >> 12) & 0x7;
+
+	return (code & RV_INSN_OPCODE_MASK) == RVG_OPCODE_AMO &&
+	       funct5 == RVG_AMO_FUNCT5_SC &&
+	       (funct3 == RVG_FUNCT3_AMO_W || funct3 == RVG_FUNCT3_AMO_D);
+}
+
 /* special case to catch _any_ system instruction */
 static __always_inline bool riscv_insn_is_system(u32 code)
 {
diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
index 433d9035b..e6a889467 100644
--- a/arch/riscv/kernel/probes/decode-insn.c
+++ b/arch/riscv/kernel/probes/decode-insn.c
@@ -23,6 +23,8 @@ riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)
 	 */
 	RISCV_INSN_REJECTED(system,		insn);
 	RISCV_INSN_REJECTED(fence,		insn);
+	RISCV_INSN_REJECTED(lr,			insn);
+	RISCV_INSN_REJECTED(sc,			insn);
 
 	/*
 	 * Simulate instructions list:
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index a76eab297..8f20f1535 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -7,11 +7,13 @@
 #include <linux/slab.h>
 #include <linux/stop_machine.h>
 #include <linux/vmalloc.h>
+#include <linux/unaligned.h>
 #include <asm/ptrace.h>
 #include <linux/uaccess.h>
 #include <asm/sections.h>
 #include <asm/cacheflush.h>
 #include <asm/bug.h>
+#include <asm/insn.h>
 #include <asm/text-patching.h>
 
 #include "decode-insn.h"
@@ -49,35 +51,72 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)
 	post_kprobe_handler(p, kcb, regs);
 }
 
-static bool __kprobes arch_check_kprobe(unsigned long addr)
+/*
+ * A trap taken in the middle of an LR/SC sequence clears the load
+ * reservation, so an SC following the probed instruction would always
+ * fail and the enclosing retry loop would re-enter the breakpoint.
+ * Reject probes inside such a sequence.
+ *
+ * RISC-V instruction boundaries cannot be recovered by walking
+ * backwards - a 32-bit instruction whose upper halfword looks like a
+ * compressed instruction is ambiguous - so walk forward from the
+ * enclosing symbol, a known instruction boundary, up to the probe address
+ * and track whether an LR is still outstanding.  A Zalrsc-constrained
+ * LR/SC retry loop holds at most 16 instructions, so an LR more than
+ * that many instructions ahead of the probe cannot start a sequence
+ * containing it.  Count instructions, not bytes: compressed
+ * instructions would make a byte window ambiguous.
+ */
+#define MAX_LRSC_GAP	16
+
+/*
+ * Verify that @addr points at the start of an instruction and is not
+ * inside an LR/SC sequence.  Returns 0 on success, -EILSEQ if @addr is
+ * not an instruction boundary, or -EINVAL if the probe would land
+ * inside an LR/SC sequence.
+ */
+static int __kprobes arch_check_kprobe(unsigned long addr)
 {
-	unsigned long tmp, offset;
+	unsigned long pc, offset;
+	int outstanding_lr = -1;
 
-	/* start iterating at the closest preceding symbol */
 	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
-		return false;
+		return -EILSEQ;
 
-	tmp = addr - offset;
+	/* pc starts at the closest preceding symbol, a known instruction boundary */
+	pc = addr - offset;
 
-	while (tmp <= addr) {
-		if (tmp == addr)
-			return true;
+	while (pc < addr) {
+		u32 insn = get_unaligned((u32 *)pc);
+		unsigned int len = GET_INSN_LENGTH(insn);
 
-		tmp += GET_INSN_LENGTH(*(u16 *)tmp);
+		if (riscv_insn_is_lr(insn))
+			outstanding_lr = 0;
+		else if (riscv_insn_is_sc(insn))
+			outstanding_lr = -1;
+		else if (outstanding_lr >= 0 && ++outstanding_lr > MAX_LRSC_GAP)
+			outstanding_lr = -1;
+
+		pc += len;
 	}
 
-	return false;
+	if (pc != addr)
+		return -EILSEQ;
+
+	return outstanding_lr >= 0 ? -EINVAL : 0;
 }
 
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	u16 *insn = (u16 *)p->addr;
+	int ret;
 
 	if ((unsigned long)insn & 0x1)
 		return -EILSEQ;
 
-	if (!arch_check_kprobe((unsigned long)p->addr))
-		return -EILSEQ;
+	ret = arch_check_kprobe((unsigned long)p->addr);
+	if (ret)
+		return ret;
 
 	/* copy instruction */
 	p->opcode = (kprobe_opcode_t)(*insn++);
-- 
2.34.1

[PATCH v5 2/2] riscv: kprobes: add KUnit test for LR/SC sequence rejection

[Xiaofeng Yuan]

Extend the kprobes KUnit test with a function containing an LR/SC
sequence and assert that register_kprobe() accepts a probe placed right
before or right after the sequence, and rejects a probe placed on the
LR, on an instruction inside the sequence, or on the SC.

Signed-off-by: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
---
 .../kernel/tests/kprobes/test-kprobes-asm.S   | 24 +++++++++++++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.c | 26 +++++++++++++++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.h |  3 +++
 3 files changed, 53 insertions(+)

diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S b/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
index 15e50934b..6625a960c 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
@@ -100,6 +100,21 @@ test_kprobes_branch_addr8:
 	.option pop
 SYM_FUNC_END(test_kprobes_branch)
 
+SYM_FUNC_START(test_kprobes_lrsc)
+.Llrsc_before:
+	nop
+1:
+.Llrsc_lr:
+	lr.w t0, 0(x0)
+.Llrsc_inside:
+	addi t0, t0, 1
+.Llrsc_sc:
+	sc.w t0, t0, 0(x0)
+	bnez t0, 1b
+.Llrsc_after:
+	ret
+SYM_FUNC_END(test_kprobes_lrsc)
+
 #ifdef CONFIG_RISCV_ISA_C
 
 SYM_FUNC_START(test_kprobes_c_j)
@@ -253,3 +268,12 @@ SYM_DATA_START(test_kprobes_functions)
 #endif /* CONFIG_RISCV_ISA_C */
 	RISCV_PTR 0
 SYM_DATA_END(test_kprobes_functions)
+
+.section .rodata
+SYM_DATA_START(test_kprobes_lrsc_offsets)
+	RISCV_PTR .Llrsc_before
+	RISCV_PTR .Llrsc_lr
+	RISCV_PTR .Llrsc_inside
+	RISCV_PTR .Llrsc_sc
+	RISCV_PTR .Llrsc_after
+SYM_DATA_END(test_kprobes_lrsc_offsets)
diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes.c b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
index 027424a3f..272753756 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes.c
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
@@ -43,8 +43,34 @@ static void test_kprobe_riscv(struct kunit *test)
 	kfree(kp);
 }
 
+static void test_kprobe_lrsc(struct kunit *test)
+{
+	/*
+	 * test_kprobes_lrsc_offsets[] points, in order, at an instruction
+	 * right before an LR/SC sequence, the LR itself, an instruction
+	 * inside the sequence, the SC, and an instruction right after it.
+	 */
+	bool reject[] = { false, true, true, true, false };
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(reject); i++) {
+		struct kprobe kp = {};
+
+		kp.pre_handler = kprobe_dummy_handler;
+		kp.addr = (kprobe_opcode_t *)test_kprobes_lrsc_offsets[i];
+
+		if (reject[i]) {
+			KUNIT_EXPECT_LT(test, register_kprobe(&kp), 0);
+		} else {
+			KUNIT_EXPECT_EQ(test, register_kprobe(&kp), 0);
+			unregister_kprobe(&kp);
+		}
+	}
+}
+
 static struct kunit_case kprobes_testcases[] = {
 	KUNIT_CASE(test_kprobe_riscv),
+	KUNIT_CASE(test_kprobe_lrsc),
 	{}
 };
 
diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes.h b/arch/riscv/kernel/tests/kprobes/test-kprobes.h
index 537f44aa9..65a8e7217 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes.h
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.h
@@ -19,6 +19,9 @@ extern void *test_kprobes_addresses[];
 /* array of functions that return KPROBE_TEST_MAGIC */
 extern long (*test_kprobes_functions[])(void);
 
+/* offsets of the probe points inside test_kprobes_lrsc() */
+extern void *test_kprobes_lrsc_offsets[];
+
 #endif /* __ASSEMBLER__ */
 
 #endif /* TEST_KPROBES_H */
-- 
2.34.1

Re: [PATCH v5 2/2] riscv: kprobes: add KUnit test for LR/SC sequence rejection

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] kprobes: stack-allocated kprobe leak and UAF on unexpected test failure
--

commit 10b8cab517db949a43dcca583dbcd1f20f6d68e8
Author: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>

riscv: kprobes: add KUnit test for LR/SC sequence rejection

This commit extends the kprobes KUnit test to verify that register_kprobe()
correctly handles an LR/SC sequence. It asserts that probes are accepted
around the sequence but rejected if placed directly on the LR, SC, or any
instruction inside the sequence.

> diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes.c b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
> index 027424a3ff7b1..2727537569739 100644
> --- a/arch/riscv/kernel/tests/kprobes/test-kprobes.c
> +++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.c

[ ... ]

> +static void test_kprobe_lrsc(struct kunit *test)
> +{
> +	/*
> +	 * test_kprobes_lrsc_offsets[] points, in order, at an instruction
> +	 * right before an LR/SC sequence, the LR itself, an instruction
> +	 * inside the sequence, the SC, and an instruction right after it.
> +	 */
> +	bool reject[] = { false, true, true, true, false };
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(reject); i++) {
> +		struct kprobe kp = {};
> +
> +		kp.pre_handler = kprobe_dummy_handler;
> +		kp.addr = (kprobe_opcode_t *)test_kprobes_lrsc_offsets[i];
> +
> +		if (reject[i]) {
> +			KUNIT_EXPECT_LT(test, register_kprobe(&kp), 0);
> +		} else {
> +			KUNIT_EXPECT_EQ(test, register_kprobe(&kp), 0);
> +			unregister_kprobe(&kp);
> +		}

[Severity: High]
Does this leave the stack-allocated kprobe registered if register_kprobe() 
unexpectedly succeeds? 

In the rejection path where reject[i] is true, KUNIT_EXPECT_LT() will log 
a test failure if register_kprobe(&kp) returns 0, but execution will
continue. Because unregister_kprobe(&kp) is only called in the else block, 
the kprobe remains in the global kprobe list.

Since struct kprobe kp is allocated on the stack within the loop, advancing
to the next iteration zeroes out the reused stack memory. When the function 
eventually returns, the stack frame is popped. This would leave a dangling
pointer in the kprobes subsystem, causing a use-after-free or kernel panic 
on subsequent kprobe operations or interrupts.

Should there be a check to unregister the kprobe if the registration 
unexpectedly succeeds in the reject path?

> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014094415.374-1-yuanxiaofeng@eswincomputing.com?part=2