riscv: probes: reject probes inside an LR/SC sequence
PATCH 1/1 riscv: probes: reject probes inside an LR/SC sequence
A kprobe or uprobe replaces an instruction with a breakpoint. The trap clears the hart's reservation, so if the breakpoint sits on the lr, on the sc, or on any instruction between them, the sc fails, the loop branches back to the lr and the probe fires again. The sequence never completes.
This has always been possible on RISC-V, because cmpxchg and the conditional atomics are LR/SC loops. arm64 has the same problem and rejects probes inside an exclusive-load/store-exclusive region.
Teach the shared instruction decoder to reject lr and sc themselves. This covers both kprobes and uprobes.
For the instructions between them, use the rules for a constrained LR/SC sequence: at most 16 instructions between lr and sc, all base integer instructions other than loads, stores, JALR, FENCE, SYSTEM and backward jumps or branches, or their compressed forms. Scan forward from the probed instruction. Reaching an sc through permitted instructions only means the probe sits inside a sequence; any other instruction, an lr, the end of the window or the end of the readable text means it does not. Only a sequence that follows these rules is guaranteed to make progress, so a probe inside any other one is already on unsupported ground. Scanning forward rather than tracking state from the start of the function also keeps an lr that has no sc, such as the one in __cmpwait() before wrs.nto, from affecting probes later in the same function.
kprobes scan the kernel text up to the end of the enclosing symbol. uprobes have only the probed instruction copied, so they read the text that follows it from the task, which the caller's mmap_lock allows.
---
This commit was submitted as patch 1 of the series [1]. Detaching it from that series with slightly edited commit message, because it is not depending on other stuff.
[1] https://patch.msgid.link/[email protected]
--- arch/riscv/include/asm/insn.h | 28 ++++++++++ arch/riscv/kernel/probes/decode-insn.c | 94 ++++++++++++++++++++++++++++++++++ arch/riscv/kernel/probes/decode-insn.h | 7 +++ arch/riscv/kernel/probes/kprobes.c | 15 ++++-- arch/riscv/kernel/probes/uprobes.c | 31 +++++++++++ 5 files changed, 172 insertions(+), 3 deletions(-)
arch/riscv/include/asm/insn.h +28 -0
@@ -10,11 +10,16 @@#define RV_INSN_FUNCT3_MASK GENMASK(14, 12)#define RV_INSN_FUNCT3_OPOFF 12 +#define RV_INSN_FUNCT5_MASK GENMASK(31, 27) +#define RV_INSN_FUNCT5_OPOFF 27 +#define RV_INSN_FUNCT7_MASK GENMASK(31, 25) +#define RV_INSN_FUNCT7_OPOFF 25#define RV_INSN_OPCODE_MASK GENMASK(6, 0)#define RV_INSN_OPCODE_OPOFF 0#define RV_INSN_FUNCT12_OPOFF 20#define RV_ENCODE_FUNCT3 (f_) (RVG_FUNCT3_##f_ << RV_INSN_FUNCT3_OPOFF) +#define RV_ENCODE_FUNCT5 (f_) (RVG_FUNCT5_##f_ << RV_INSN_FUNCT5_OPOFF)#define RV_ENCODE_FUNCT12 (f_) (RVG_FUNCT12_##f_ << RV_INSN_FUNCT12_OPOFF)/* The bit field of immediate value in I-type instruction */
@@ -136,7 +141,13 @@/* parts of opcode for RVG*/ #define RVG_OPCODE_FENCE 0x0f +#define RVG_OPCODE_OP_IMM 0x13#define RVG_OPCODE_AUIPC 0x17 +#define RVG_OPCODE_OP_IMM_32 0x1b +#define RVG_OPCODE_AMO 0x2f +#define RVG_OPCODE_OP 0x33 +#define RVG_OPCODE_LUI 0x37 +#define RVG_OPCODE_OP_32 0x3b#define RVG_OPCODE_BRANCH 0x63#define RVG_OPCODE_JALR 0x67#define RVG_OPCODE_JAL 0x6f
@@ -188,6 +199,17 @@#define RVG_FUNCT12_EBREAK 0x1#define RVG_FUNCT12_SRET 0x102 +/* parts of funct5 code for A extension */ +#define RVG_FUNCT5_LR 0x02 +#define RVG_FUNCT5_SC 0x03 + +/* +* The AMO funct3 field holds the access width: 010 for .w, 011 for .d. +* Matching bits 14:13 alone accepts both and nothing else. +*/ +#define RVG_AMO_WIDTH_MASK GENMASK(14, 13) +#define RVG_AMO_WIDTH_WD BIT(13) +#define RVG_MATCH_AUIPC (RVG_OPCODE_AUIPC)#define RVG_MATCH_JALR (RV_ENCODE_FUNCT3(JALR) | RVG_OPCODE_JALR)#define RVG_MATCH_JAL (RVG_OPCODE_JAL)
@@ -200,6 +222,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_AMO_WIDTH_WD | RVG_OPCODE_AMO) +#define RVG_MATCH_SC (RV_ENCODE_FUNCT5(SC) | RVG_AMO_WIDTH_WD | RVG_OPCODE_AMO)#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 +251,8 @@#define RVC_MASK_C_EBREAK 0xffff#define RVG_MASK_EBREAK 0xffffffff#define RVG_MASK_SRET 0xffffffff +#define RVG_MASK_LR (RV_INSN_FUNCT5_MASK | RVG_AMO_WIDTH_MASK | RV_INSN_OPCODE_MASK) +#define RVG_MASK_SC (RV_INSN_FUNCT5_MASK | RVG_AMO_WIDTH_MASK | RV_INSN_OPCODE_MASK)#define __INSN_LENGTH_MASK _UL(0x3)#define __INSN_LENGTH_GE_32 _UL(0x3)
@@ -262,6 +288,8 @@ __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 ); +__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)
arch/riscv/kernel/probes/decode-insn.c +94 -0
111 lines not on this page · raw message
arch/riscv/kernel/probes/decode-insn.h +7 -0
@@ -15,4 +15,11 @@ enum probe_insn {enum probe_insn __kprobes riscv_probe_decode_insn (probe_opcode_t *addr,struct arch_probe_insn *asi); +/* A constrained LR/SC sequence has at most this many instructions between lr and sc */ +#define RISCV_LRSC_MAX_BODY 16 +/* Bytes from the probed instruction through the farthest possible sc */ +#define RISCV_LRSC_SCAN_BYTES ((RISCV_LRSC_MAX_BODY + 1) * 4) + +bool __kprobes riscv_probe_in_lrsc_body (const u16 *buf,size_t len); +#endif /* _RISCV_KERNEL_KPROBES_DECODE_INSN_H */
arch/riscv/kernel/probes/kprobes.c +12 -3
@@ -49,19 +49,28 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)post_kprobe_handler (p, kcb, regs); } +/* +* Walk the instructions from the enclosing symbol up to addr to check that +* addr is an instruction boundary. Then reject a probe inside an LR/SC +* sequence: the breakpoint trap drops the reservation, so the SC would fail +* and the loop would hit the probe again on every retry. The sequence is +* recognised by a forward scan with the constrained-sequence rules, as for +* uprobes, so an lr that is not followed by an sc (as in __cmpwait()) does +* not taint the rest of the function. +*/ static bool __kprobes arch_check_kprobe (unsigned long addr) { -unsigned long tmp, offset; +unsigned long tmp, size, offset;/* start iterating at the closest preceding symbol */ -if (!kallsyms_lookup_size_offset (addr,NULL , &offset)) +if (!kallsyms_lookup_size_offset (addr, &size, &offset))return false; tmp = addr - offset;while (tmp <= addr) {if (tmp == addr) -return true; +return !riscv_probe_in_lrsc_body ((u16 *)addr, size - offset); tmp +=GET_INSN_LENGTH (*(u16 *)tmp); }
arch/riscv/kernel/probes/uprobes.c +31 -0
@@ -28,6 +28,34 @@ unsigned long uprobe_get_swbp_addr(struct pt_regs *regs)return instruction_pointer (regs); } +/* +* Is the instruction at addr inside a constrained LR/SC sequence? Only the +* probed instruction was copied, so read the text that follows it from the +* task. The caller holds mmap_lock, as uprobe_write_opcode() requires too. +* Text that cannot be read ends the scan early, which accepts the probe. +*/ +static bool uprobe_in_lrsc_body (struct mm_struct *mm,unsigned long addr) +{ +u16 buf[RISCV_LRSC_SCAN_BYTES /sizeof (u16)]; +struct vm_area_struct *vma; +struct page *page; +size_t got =0 ; +int len; + +while (got <sizeof (buf)) { + len =min_t (size_t,sizeof (buf) - got, +PAGE_SIZE -offset_in_page (addr + got)); + page =get_user_page_vma_remote (mm, addr + got,FOLL_FORCE , &vma); +if (IS_ERR (page)) +break ; +uprobe_copy_from_page (page, addr + got, (void *)buf + got, len); +put_page (page); + got += len; + } + +return riscv_probe_in_lrsc_body (buf, got); +} +int arch_uprobe_analyze_insn (struct arch_uprobe *auprobe,struct mm_struct *mm,unsigned long addr) {
@@ -53,6 +81,9 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, struct mm_struct *mm,return -EINVAL ; } +if (uprobe_in_lrsc_body (mm, addr)) +return -EINVAL ; +return 0 ; }
---
base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
change-id: 20261111-probes-in-lrsc-4ccde6cbf436
Best regards,
Signature · 1 line
Vladimir Kondratiev <[email protected]>
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue to consider: - [Medium] riscv: probes: arch_prepare_kprobe() returns -EILSEQ for LR/SC probes It also noticed 1 related…
Thank you for your contribution! Sashiko AI review found 1 potential issue to consider:
- [Medium] riscv: probes: arch_prepare_kprobe() returns -EILSEQ for LR/SC probes
It also noticed 1 related pre-existing issue this patch does not introduce:
- [High] riscv/kprobes: out-of-bounds read in riscv_probe_decode_insn()
[1]
--
commit 231a5b2589b5c10fae9eb5a24fcd30822aa304a2
Author: Vladimir Kondratiev <[email protected]>
riscv: probes: reject probes inside an LR/SC sequence
This commit rejects kprobes and uprobes placed on lr or sc instructions or inside the body of a constrained LR/SC sequence so breakpoint traps do not repeatedly clear the reservation.
Quoted 4 lines
arch/riscv/kernel/probes/decode-insn.c +0
-0
[ ... ]
arch/riscv/kernel/probes/decode-insn.c riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api) · line 119
Quoted 10 lines
arch/riscv/kernel/probes/decode-insn.c +6
-0@@ -23,6 +111,12 @@ riscv_probe_decode_insn(probe_opcode_t *addr, struct arch_probe_insn *api)*/ RISCV_INSN_REJECTED (system, insn);RISCV_INSN_REJECTED (fence, insn); +/* +* The breakpoint trap drops the reservation, so a probe on either +* end of an LR/SC sequence makes the SC fail on every retry. +*/ +RISCV_INSN_REJECTED (lr, insn); +RISCV_INSN_REJECTED (sc, insn);
[Severity: High]
This problem wasn't introduced by this patch, but can riscv_probe_decode_insn() trigger an out-of-bounds page fault [1] when probing a 16-bit compressed instruction at the end of a page?
arch_prepare_kprobe() safely reads 16-bit parcels into p->opcode, but then passes p->addr to riscv_probe_decode_insn(), which unconditionally dereferences *addr as a 32-bit probe_opcode_t:
arch/riscv/kernel/probes/kprobes.c:arch_prepare_kprobe() {
...
/* copy instruction */
p->opcode = (kprobe_opcode_t)(*insn++);
if (GET_INSN_LENGTH(p->opcode) == 4)
p->opcode |= (kprobe_opcode_t)(*insn) << 16;
/* decode instruction */
switch (riscv_probe_decode_insn(p->addr, &p->ainsn.api)) {
...
}If a 16-bit instruction sits in the last two bytes of a mapped page next to an unmapped page, the 32-bit read of *addr crosses the page boundary and faults. Passing &p->opcode instead of p->addr to riscv_probe_decode_insn() (as arch_uprobe_analyze_insn() already does) would avoid reading past the instruction.
[ ... ]
Quoted 23 lines
arch/riscv/kernel/probes/kprobes.c +10
-1@@ -49,19 +49,28 @@ static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs)post_kprobe_handler (p, kcb, regs); } +/* +* Walk the instructions from the enclosing symbol up to addr to check that +* addr is an instruction boundary. Then reject a probe inside an LR/SC +* sequence: the breakpoint trap drops the reservation, so the SC would fail +* and the loop would hit the probe again on every retry. The sequence is +* recognised by a forward scan with the constrained-sequence rules, as for +* uprobes, so an lr that is not followed by an sc (as in __cmpwait()) does +* not taint the rest of the function. +*/ static bool __kprobes arch_check_kprobe (unsigned long addr) { -unsigned long tmp, offset; +unsigned long tmp, size, offset;/* start iterating at the closest preceding symbol */
arch/riscv/kernel/probes/kprobes.c static void __kprobes arch_simulate_insn(struct kprobe *p, struct pt_regs *regs) · line 73
Quoted 10 lines
arch/riscv/kernel/probes/kprobes.c +2
-2-if (!kallsyms_lookup_size_offset (addr,NULL , &offset)) +if (!kallsyms_lookup_size_offset (addr, &size, &offset))return false; tmp = addr - offset;while (tmp <= addr) {if (tmp == addr) -return true; +return !riscv_probe_in_lrsc_body ((u16 *)addr, size - offset);
[Severity: Medium]
Does rejecting LR/SC body and sc instructions inside arch_check_kprobe() cause arch_prepare_kprobe() to return -EILSEQ instead of -EINVAL?
arch_prepare_kprobe() returns -EILSEQ whenever arch_check_kprobe() returns false, and trace_kprobe_create_internal() maps -EILSEQ to BAD_INSN_BNDRY ("Probe point is not an instruction boundary"). Because riscv_probe_in_lrsc_body() also returns true at n = 0 when the probed instruction itself is sc, kprobes on sc or inside an LR/SC body fail with -EILSEQ and report a bad instruction boundary, whereas kprobes on lr and uprobes return -EINVAL (leaving RISCV_INSN_REJECTED(sc, insn) in riscv_probe_decode_insn() unreachable for kprobes).
[1] https://sashiko.dev/#/bug/linux-812af091-20d1-4915-97a6-741f51e07022
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1