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

[PATCH v4 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
inside an LR/SC sequence:

  - patch 1 detects the sequences (insn.h decoding helpers, reject list
    in decode-insn.c, and a forward scan from the function start in
    kprobes.c).
  - patch 2 adds a KUnit test that registers a probe inside and right
    after a hand-written LR/SC loop and checks rejection/acceptance.

Changes in v4:
  - v3 walked backwards from the probe to find an open LR.  That was
    wrong twice: the LR/SC state was toggled in the reverse order, and
    worse, a backward walk is fundamentally ambiguous in RISC-V (the
    halfword at addr-2 may be a compressed instruction or the upper
    half of a 32-bit instruction, and the length bits cannot be
    trusted).  v4 walks forward from the function start instead, a
    known instruction boundary.
  - document the kallsyms_lookup_size_offset() offset-0 caveat in the
    commit message, and test it with local (.L) labels so probe
    addresses never coincide with kallsyms symbols inside the sequence.
  - the KUnit test (patch 2) is new in v4.  No earlier version had a
    dedicated LR/SC test, which is why the backward-scan bugs went
    unnoticed.

Link: https://lore.kernel.org/linux-riscv/REPLACE-WITH-V3-MSGID/

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

 arch/riscv/include/asm/insn.h                 | 18 +++++++
 arch/riscv/kernel/probes/decode-insn.c        |  2 +
 arch/riscv/kernel/probes/kprobes.c            | 54 +++++++++++++++++++
 .../kernel/tests/kprobes/test-kprobes-asm.S   | 19 +++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.c | 19 +++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.h |  6 +++
 6 files changed, 118 insertions(+)

-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH v4 1/2] riscv: probes: reject kprobes 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 would always
fail and the enclosing retry loop would re-enter the breakpoint,
livelocking the CPU.

Reject probing the LR/SC instructions themselves, and reject probing
any address that lies inside an LR/SC sequence.  A constrained LR/SC
loop (Zalrsc) is at most 16 instructions contained in a 64-byte region.
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 function start, a
known instruction boundary, up to the probe address and reject the probe
if an LR is still outstanding when it is reached.

kallsyms_lookup_size_offset() returns the offset of the probe from the
start of the enclosing symbol, which is used to find the function start.
If the probe address exactly matched a kallsyms symbol, that offset would
be 0 and the forward scan would be skipped, silently missing the LR/SC
sequence.  To avoid this, global labels should not be placed at interior
instructions of an LR/SC sequence.  In practice this is not a
restriction: LR/SC sequences are tight retry loops and generally do not
carry global labels inside them, so the enclosing function start is
resolved correctly and the forward scan proceeds as intended.

Signed-off-by: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
---
v2: address review comments from Nam Cao:
    - clarify that the 64-byte scan bound still holds with the C extension
    - document that scan_start need not be an instruction boundary
    - reuse the decoded insn for GET_INSN_LENGTH(), drop the (u16 *) cast
v3: rework the scan per Nam Cao's review:
    - walk 16 instructions backwards from the probe instead of scanning
      forward from the function entry, decoding instruction length from
      the low two bits of each instruction
    - drop the kallsyms_lookup_size_offset() dependency
    - dereference the instructions directly since probes always sit on
      the resident kernel text mapping
    - use get_unaligned() for the backward walk, which may cross 2-byte
      instruction boundaries and read a 32-bit instruction from a
      halfword-aligned address
v4: fix two bugs found during review and testing:
    - the backward walk toggled the LR/SC state in the wrong order: for
      "lr; insn; sc; probe" it saw the sc first and then the lr, wrongly
      reporting the probe as inside the sequence
    - the backward walk is fundamentally ambiguous in RISC-V: when walking
      backwards, the halfword at (addr - 2) could be either a 16-bit
      compressed instruction (low bits are opcode) or the upper half of a
      32-bit instruction (low bits are part of the immediate/rd fields),
      and there is no way to distinguish the two, so instruction length
      cannot be determined and boundaries cannot be recovered
    So walk forward instead: use kallsyms_lookup_size_offset() to find
    the function start, a known instruction boundary, then walk forward
    to the probe tracking whether an LR is still outstanding.
---
 arch/riscv/include/asm/insn.h          | 18 +++++++++
 arch/riscv/kernel/probes/decode-insn.c |  2 +
 arch/riscv/kernel/probes/kprobes.c     | 54 ++++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/arch/riscv/include/asm/insn.h b/arch/riscv/include/asm/insn.h
index c3005573e8..d7d85b1b84 100644
--- a/arch/riscv/include/asm/insn.h
+++ b/arch/riscv/include/asm/insn.h
@@ -13,9 +13,12 @@
 #define RV_INSN_OPCODE_MASK	GENMASK(6, 0)
 #define RV_INSN_OPCODE_OPOFF	0
 #define RV_INSN_FUNCT12_OPOFF	20
+#define RVG_FUNCT5_MASK		GENMASK(31, 27)
+#define RVG_FUNCT5_OPOFF	27
 
 #define RV_ENCODE_FUNCT3(f_)	(RVG_FUNCT3_##f_ << RV_INSN_FUNCT3_OPOFF)
 #define RV_ENCODE_FUNCT12(f_)	(RVG_FUNCT12_##f_ << RV_INSN_FUNCT12_OPOFF)
+#define RV_ENCODE_FUNCT5(f_)	(RVG_FUNCT5_##f_ << RVG_FUNCT5_OPOFF)
 
 /* The bit field of immediate value in I-type instruction */
 #define RV_I_IMM_SIGN_OPOFF	31
@@ -137,6 +140,7 @@
 /* parts of opcode for RVG*/
 #define RVG_OPCODE_FENCE	0x0f
 #define RVG_OPCODE_AUIPC	0x17
+#define RVG_OPCODE_LRSC		0x2f
 #define RVG_OPCODE_BRANCH	0x63
 #define RVG_OPCODE_JALR		0x67
 #define RVG_OPCODE_JAL		0x6f
@@ -176,6 +180,9 @@
 #define RVG_FUNCT3_BLTU		0x6
 #define RVG_FUNCT3_BGEU		0x7
 
+#define RVG_FUNCT5_LR		0x02
+#define RVG_FUNCT5_SC		0x03
+
 /* parts of funct3 code for C extension*/
 #define RVC_FUNCT3_C_BEQZ	0x6
 #define RVC_FUNCT3_C_BNEZ	0x7
@@ -200,6 +207,8 @@
 #define RVG_MATCH_BGEU		(RV_ENCODE_FUNCT3(BGEU) | RVG_OPCODE_BRANCH)
 #define RVG_MATCH_EBREAK	(RV_ENCODE_FUNCT12(EBREAK) | RVG_OPCODE_SYSTEM)
 #define RVG_MATCH_SRET		(RV_ENCODE_FUNCT12(SRET) | RVG_OPCODE_SYSTEM)
+#define RVG_MATCH_LR		(RV_ENCODE_FUNCT5(LR) | RVG_OPCODE_LRSC)
+#define RVG_MATCH_SC		(RV_ENCODE_FUNCT5(SC) | RVG_OPCODE_LRSC)
 #define RVC_MATCH_C_BEQZ	(RVC_ENCODE_FUNCT3(C_BEQZ) | RVC_OPCODE_C1)
 #define RVC_MATCH_C_BNEZ	(RVC_ENCODE_FUNCT3(C_BNEZ) | RVC_OPCODE_C1)
 #define RVC_MATCH_C_J		(RVC_ENCODE_FUNCT3(C_J) | RVC_OPCODE_C1)
@@ -227,6 +236,8 @@
 #define RVC_MASK_C_EBREAK	0xffff
 #define RVG_MASK_EBREAK		0xffffffff
 #define RVG_MASK_SRET		0xffffffff
+#define RVG_MASK_LR		(RVG_FUNCT5_MASK | GENMASK(14, 14) | RV_INSN_OPCODE_MASK)
+#define RVG_MASK_SC		(RVG_FUNCT5_MASK | GENMASK(14, 14) | RV_INSN_OPCODE_MASK)
 
 #define __INSN_LENGTH_MASK	_UL(0x3)
 #define __INSN_LENGTH_GE_32	_UL(0x3)
@@ -262,6 +273,13 @@ __RISCV_INSN_FUNCS(c_ebreak, RVC_MASK_C_EBREAK, RVC_MATCH_C_EBREAK)
 __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 (Zalrsc, opcode 0x2f).  funct3 selects the operand size: 000 (W)
+ * and 011 (D) on RV64; bit 14 is clear for both, so it is used to match
+ * either size.  The .aq/.rl bits (26:25) and rd are ignored.
+ */
+__RISCV_INSN_FUNCS(lr, RVG_MASK_LR, RVG_MATCH_LR)
+__RISCV_INSN_FUNCS(sc, RVG_MASK_SC, RVG_MATCH_SC)
 
 /* 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 65d9590bfb..eae393ef58 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 9e2afabf94..f858426cd3 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -9,10 +9,12 @@
 #include <linux/vmalloc.h>
 #include <asm/ptrace.h>
 #include <linux/uaccess.h>
+#include <linux/unaligned.h>
 #include <asm/sections.h>
 #include <asm/cacheflush.h>
 #include <asm/bug.h>
 #include <asm/text-patching.h>
+#include <asm/insn.h>
 
 #include "decode-insn.h"
 
@@ -69,6 +71,55 @@ static bool __kprobes arch_check_kprobe(unsigned long addr)
 	return false;
 }
 
+/*
+ * 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.
+ *
+ * A constrained LR/SC loop (Zalrsc) is at most 16 instructions and
+ * must be contained in a 64-byte contiguous region of memory, so only
+ * instructions within that window preceding the probe can open a
+ * sequence containing it.  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 function start, a known instruction boundary, up to
+ * the probe address and track whether an LR is still outstanding.
+ */
+#define MAX_ATOMIC_CONTEXT_SIZE	64
+
+static bool __kprobes riscv_probe_insn_in_atomic(unsigned long addr)
+{
+	unsigned long start, offset, pc;
+	bool in_atomic = false;
+
+	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
+		return false;
+
+	start = addr - offset;
+	pc = start;
+
+	while (pc < addr) {
+		u16 halfword = *(u16 *)pc;
+		unsigned int len = (halfword & 0x3) == 0x3 ? 4 : 2;
+
+		if (addr - pc <= MAX_ATOMIC_CONTEXT_SIZE) {
+			if (len == 4) {
+				u32 insn = get_unaligned((u32 *)pc);
+
+				if (riscv_insn_is_lr(insn))
+					in_atomic = true;
+				else if (riscv_insn_is_sc(insn))
+					in_atomic = false;
+			}
+		}
+
+		pc += len;
+	}
+
+	return in_atomic;
+}
+
 int __kprobes arch_prepare_kprobe(struct kprobe *p)
 {
 	u16 *insn = (u16 *)p->addr;
@@ -79,6 +130,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
 	if (!arch_check_kprobe((unsigned long)p->addr))
 		return -EILSEQ;
 
+	if (riscv_probe_insn_in_atomic((unsigned long)p->addr))
+		return -EINVAL;
+
 	/* copy instruction */
 	p->opcode = (kprobe_opcode_t)(*insn++);
 	if (GET_INSN_LENGTH(p->opcode) == 4)
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Xiaofeng Yuan]

Add a KUnit test that probes an instruction inside and right after an
LR/SC sequence, and check that the former is rejected while the latter
is accepted.

The probe points are declared as local (.L) labels, which are filtered
out of the kallsyms symbol table, so kallsyms_lookup_size_offset()
always resolves to the enclosing function and the forward scan starts
from the correct function boundary.  A probe address that coincided
with a kallsyms symbol would make the offset 0 and skip the scan
entirely.

Signed-off-by: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
---
v4: New in v4: add a dedicated KUnit test for the LR/SC rejection path.
    The existing test_kprobe_riscv only probes ordinary instructions, so
    the LR/SC rejection path was never exercised and the two
    backward-scan bugs went unnoticed.
---
 .../kernel/tests/kprobes/test-kprobes-asm.S   | 19 +++++++++++++++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.c | 19 +++++++++++++++++++
 .../riscv/kernel/tests/kprobes/test-kprobes.h |  6 ++++++
 3 files changed, 44 insertions(+)

diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S b/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
index f16deee9e0..f2c412aedd 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes-asm.S
@@ -181,6 +181,25 @@ SYM_FUNC_END(test_kprobes_c_bnez)
 
 #endif /* CONFIG_RISCV_ISA_C */
 
+SYM_FUNC_START(test_kprobes_lrsc)
+	li a0, 1
+1:
+	lr.w a1, 0(x0)
+.Llrsc_inside:
+	addi a1, a1, 1
+	sc.w a2, a1, 0(x0)
+	bnez a2, 1b
+.Llrsc_after:
+	li a0, KPROBE_TEST_MAGIC
+	ret
+SYM_FUNC_END(test_kprobes_lrsc)
+
+.section .rodata
+SYM_DATA_START(test_kprobes_lrsc_offsets)
+	RISCV_PTR .Llrsc_inside - test_kprobes_lrsc
+	RISCV_PTR .Llrsc_after - test_kprobes_lrsc
+SYM_DATA_END(test_kprobes_lrsc_offsets)
+
 .section .rodata
 SYM_DATA_START(test_kprobes_addresses)
 	RISCV_PTR test_kprobes_add_addr1
diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes.c b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
index 027424a3ff..25bd103e20 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes.c
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
@@ -43,8 +43,27 @@ static void test_kprobe_riscv(struct kunit *test)
 	kfree(kp);
 }
 
+static void test_kprobe_lrsc(struct kunit *test)
+{
+	struct kprobe kp = {};
+
+	kp.pre_handler = kprobe_dummy_handler;
+
+	/* a probe inside an LR/SC sequence must be rejected */
+	kp.addr = (kprobe_opcode_t *)((unsigned long)test_kprobes_lrsc +
+				      test_kprobes_lrsc_offsets[0]);
+	KUNIT_EXPECT_LT(test, register_kprobe(&kp), 0);
+
+	/* a probe right after the sequence must be accepted */
+	kp.addr = (kprobe_opcode_t *)((unsigned long)test_kprobes_lrsc +
+				      test_kprobes_lrsc_offsets[1]);
+	KUNIT_EXPECT_EQ(test, 0, register_kprobe(&kp));
+	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 537f44aa9d..58be980fdf 100644
--- a/arch/riscv/kernel/tests/kprobes/test-kprobes.h
+++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.h
@@ -19,6 +19,12 @@ extern void *test_kprobes_addresses[];
 /* array of functions that return KPROBE_TEST_MAGIC */
 extern long (*test_kprobes_functions[])(void);
 
+/* function containing an LR/SC sequence, and offsets (from its start) of
+ * the instruction inside and right after the sequence
+ */
+extern void test_kprobes_lrsc(void);
+extern unsigned long test_kprobes_lrsc_offsets[];
+
 #endif /* __ASSEMBLER__ */
 
 #endif /* TEST_KPROBES_H */
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Xiaofeng Yuan]

Hi Nam,

Gentle ping on this series. v4 replaced the ambiguous backward scan
with a forward walk from the function start and added a KUnit test;
it would be great if you could take a look when you get a chance.

Thanks,
Xiaofeng



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nam Cao]

Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com> writes:
> kallsyms_lookup_size_offset() returns the offset of the probe from the
> start of the enclosing symbol, which is used to find the function start.
> If the probe address exactly matched a kallsyms symbol, that offset would
> be 0 and the forward scan would be skipped, silently missing the LR/SC
> sequence.

...okay, that sounds like a problem.

> To avoid this, global labels should not be placed at interior
> instructions of an LR/SC sequence.

"Should not be placed" by whom? Kernel developers, compilers? And how do
we ensure that?

> In practice this is not a
> restriction: LR/SC sequences are tight retry loops and generally do not
> carry global labels inside them, so the enclosing function start is
> resolved correctly and the forward scan proceeds as intended.

"Generally" not a problem is not good enough, sorry. It either is a
problem and we must deal with it, or it is never a problem.

I am not sure if labels will be placed between LR and SC. But a label on
the LR instruction sounds entirely possible.

What confuses me is that this case does not sound difficult to
accommodate. Any reason why we should not or cannot do that?

> +/*
> + * 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.
> + *
> + * A constrained LR/SC loop (Zalrsc) is at most 16 instructions and
> + * must be contained in a 64-byte contiguous region of memory, so only
> + * instructions within that window preceding the probe can open a
> + * sequence containing it.  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 function start, a known instruction boundary, up to
> + * the probe address and track whether an LR is still outstanding.
> + */
> +#define MAX_ATOMIC_CONTEXT_SIZE	64

Doesn't this mean we can walk for more than 16 instructions and
therefore can have false rejection?

> +static bool __kprobes riscv_probe_insn_in_atomic(unsigned long addr)
> +{
> +	unsigned long start, offset, pc;
> +	bool in_atomic = false;
> +
> +	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
> +		return false;
> +
> +	start = addr - offset;
> +	pc = start;
> +
> +	while (pc < addr) {
> +		u16 halfword = *(u16 *)pc;
> +		unsigned int len = (halfword & 0x3) == 0x3 ? 4 : 2;

We already have a macro to get the instruction length, please use that.

And why do we need to separately load 16-bit 'halfword' here, and load
32-bit 'insn' later on? Can't we use 'insn' for both usage?

> +
> +		if (addr - pc <= MAX_ATOMIC_CONTEXT_SIZE) {
> +			if (len == 4) {
> +				u32 insn = get_unaligned((u32 *)pc);
> +
> +				if (riscv_insn_is_lr(insn))
> +					in_atomic = true;
> +				else if (riscv_insn_is_sc(insn))
> +					in_atomic = false;
> +			}
> +		}
> +
> +		pc += len;
> +	}
> +
> +	return in_atomic;
> +}
> +
>  int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  {
>  	u16 *insn = (u16 *)p->addr;
> @@ -79,6 +130,9 @@ int __kprobes arch_prepare_kprobe(struct kprobe *p)
>  	if (!arch_check_kprobe((unsigned long)p->addr))
>  		return -EILSEQ;
>  
> +	if (riscv_probe_insn_in_atomic((unsigned long)p->addr))
> +		return -EINVAL;
> +

arch_check_kprobe() is already doing a walk. It probably is a good idea
to merge this with that, to avoid walking twice.

Nam

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

[Nam Cao]

Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com> writes:
> +SYM_FUNC_START(test_kprobes_lrsc)
> +	li a0, 1
> +1:
> +	lr.w a1, 0(x0)

It would be nice to place this instruction right at the beginning (ditch
the "li a0, 1"), so we can test that it is not wrongly skipped.

> +.Llrsc_inside:

Shouldn't the lr.w instruction be rejected as well?

> +	addi a1, a1, 1
> +	sc.w a2, a1, 0(x0)
> +	bnez a2, 1b
> +.Llrsc_after:
> +	li a0, KPROBE_TEST_MAGIC

We don't need to return KPROBE_TEST_MAGIC for this test case.

> +	ret
> +SYM_FUNC_END(test_kprobes_lrsc)
> +
> +.section .rodata
> +SYM_DATA_START(test_kprobes_lrsc_offsets)
> +	RISCV_PTR .Llrsc_inside - test_kprobes_lrsc
> +	RISCV_PTR .Llrsc_after - test_kprobes_lrsc

Why do we do the subtraction here, only to do the reverse addition in
test_kprobe_lrsc()?

> +SYM_DATA_END(test_kprobes_lrsc_offsets)
> +
>  .section .rodata
>  SYM_DATA_START(test_kprobes_addresses)
>  	RISCV_PTR test_kprobes_add_addr1
> diff --git a/arch/riscv/kernel/tests/kprobes/test-kprobes.c b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
> index 027424a3ff..25bd103e20 100644
> --- a/arch/riscv/kernel/tests/kprobes/test-kprobes.c
> +++ b/arch/riscv/kernel/tests/kprobes/test-kprobes.c
> @@ -43,8 +43,27 @@ static void test_kprobe_riscv(struct kunit *test)
>  	kfree(kp);
>  }
>  
> +static void test_kprobe_lrsc(struct kunit *test)
> +{
> +	struct kprobe kp = {};
> +
> +	kp.pre_handler = kprobe_dummy_handler;
> +
> +	/* a probe inside an LR/SC sequence must be rejected */
> +	kp.addr = (kprobe_opcode_t *)((unsigned long)test_kprobes_lrsc +
> +				      test_kprobes_lrsc_offsets[0]);
> +	KUNIT_EXPECT_LT(test, register_kprobe(&kp), 0);
> +
> +	/* a probe right after the sequence must be accepted */
> +	kp.addr = (kprobe_opcode_t *)((unsigned long)test_kprobes_lrsc +
> +				      test_kprobes_lrsc_offsets[1]);
> +	KUNIT_EXPECT_EQ(test, 0, register_kprobe(&kp));
> +	unregister_kprobe(&kp);
> +}

We should have at least 4 checks: right before, first inside, last
inside, right after.

Nam

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[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


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv