Re: [PATCH v2 2/2] riscv: probes: reject probes inside an LR/SC sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] riscv: probes: riscv_probe_in_lrsc_body() returns false when
  probing lr
- [Low] riscv: probes: missing Fixes trailer on LR/SC probe rejection
  fix
--

commit fa1e6ee354b2fc2997caf3fb258fcb6a4fda3097
Author: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

> 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.
>
> Breakpoint can be on the lr unless it is part of the LR/SC loop.
>
> For the instructions between LR and SC, 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.
>
> Signed-off-by: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com>

[Severity: Low]
Since this fixes a livelock present since kprobes and uprobes support was
added on RISC-V, should this include Fixes tags?

  Fixes: c22b0bcb1dd0 ("riscv: Add kprobes supported")
  Fixes: 74784081aac8 ("riscv: Add uprobes supported")

[ ... ]

> diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
> index 433d9035b8882..0789754f682cb 100644
> --- a/arch/riscv/kernel/probes/decode-insn.c
> +++ b/arch/riscv/kernel/probes/decode-insn.c
> @@ -4,11 +4,102 @@
>  #include <linux/kprobes.h>
>  #include <linux/module.h>
>  #include <linux/kallsyms.h>
> +#include <linux/bug.h>
>  #include <asm/sections.h>
>  
>  #include "decode-insn.h"
>  #include "simulate-insn.h"
>  
> +/*
> + * Can insn sit between the lr and the sc of a constrained LR/SC sequence?
> + * The ISA allows base integer instructions there, except loads, stores,
> + * JALR, FENCE, SYSTEM, backward jumps and taken backward branches, plus
> + * their compressed forms. Backward branches are treated as not allowed:
> + * the retry branch follows the sc, so one inside the body is not a
> + * constrained sequence.
> + */
> +static bool __kprobes riscv_insn_is_lrsc_body(u32 insn)
> +{
> +	u32 funct3;
> +
> +	if (GET_INSN_LENGTH(insn) == 2) {
> +		switch (insn & RVC_INSN_OPCODE_MASK) {
> +		case RVC_OPCODE_C0:
> +			/* c.addi4spn; the rest of the quadrant is loads and stores */
> +			return (insn & RVC_INSN_FUNCT3_MASK) == 0;
> +		case RVC_OPCODE_C1:
> +			/* Arithmetic, c.j, c.jal and branches, bit 12 is the offset sign */
> +			if (riscv_insn_is_c_j(insn) || riscv_insn_is_c_jal(insn) ||
> +			    riscv_insn_is_c_beqz(insn) || riscv_insn_is_c_bnez(insn))
> +				return !(insn & BIT(12));
> +			return true;
> +		case RVC_OPCODE_C2:
> +			funct3 = (insn & RVC_INSN_FUNCT3_MASK) >> RVC_INSN_FUNCT3_OPOFF;
> +			if (funct3 == 0)	/* c.slli */
> +				return true;
> +			if (funct3 == 4)	/* c.mv, c.add, but not c.jr, c.jalr, c.ebreak */
> +				return !riscv_insn_is_c_jr(insn) &&
> +				       !riscv_insn_is_c_jalr(insn) &&
> +				       !riscv_insn_is_c_ebreak(insn);
> +			return false;	/* stack loads and stores */
> +		}
> +		return false;
> +	}
> +
> +	switch (insn & RV_INSN_OPCODE_MASK) {
> +	case RVG_OPCODE_OP_IMM:
> +	case RVG_OPCODE_OP_IMM_32:
> +	case RVG_OPCODE_LUI:
> +	case RVG_OPCODE_AUIPC:
> +		return true;
> +	case RVG_OPCODE_OP:
> +	case RVG_OPCODE_OP_32:
> +		/* funct7 0 or 0x20 is base I; anything else is M, B or another extension */
> +		return !((insn & RV_INSN_FUNCT7_MASK) & ~(0x20U << RV_INSN_FUNCT7_OPOFF));
> +	case RVG_OPCODE_BRANCH:
> +	case RVG_OPCODE_JAL:
> +		/* forward only, bit 31 is the offset sign */
> +		return !(insn & BIT(31));
> +	}
> +	return false;
> +}
> +
> +/*
> + * Does the instruction at the start of buf lie inside a constrained LR/SC
> + * sequence? Such a sequence holds at most RISCV_LRSC_MAX_BODY instructions
> + * between the lr and the sc, all from the set above. Walk forward from the
> + * probed instruction: reaching an sc through permitted instructions only
> + * means the probe sits in the body; anything else, including an lr that
> + * opens a new sequence, means it does not. The walk stops at the end of
> + * buf, so a short buffer only makes the answer more permissive.
> + */
> +bool __kprobes riscv_probe_in_lrsc_body(const u16 *buf, size_t len)
> +{
> +	size_t off = 0;
> +	u32 insn;
> +	int n;
> +
> +	for (n = 0; n < RISCV_LRSC_MAX_BODY + 2; n++) { // 2 for LR and SC itself
> +		if (off + 2 > len)
> +			return false;
> +		insn = buf[off / 2];
> +		if (GET_INSN_LENGTH(insn) == 4) {
> +			if (off + 4 > len)
> +				return false;
> +			insn |= (u32)buf[off / 2 + 1] << 16;
> +		}
> +		/* if probed instruction is not lr, scan one instruction less */
> +		if (n == 0 && !riscv_insn_is_lr(insn))
> +			n++;
> +		if (riscv_insn_is_sc(insn))
> +			return true;
> +		if (!riscv_insn_is_lrsc_body(insn))
> +			return false;

[Severity: High]
Does riscv_probe_in_lrsc_body() always return false when the probed
instruction at n == 0 is lr?

When riscv_insn_is_lr(insn) is true at n == 0, execution still falls
through to riscv_insn_is_lrsc_body(insn).  Because riscv_insn_is_lrsc_body()
rejects RVG_OPCODE_AMO, it returns false on the first iteration without
scanning for a matching sc, allowing probes on lr inside an LR/SC loop to
be accepted and livelock when single-stepped.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-probes-in-lrsc-v2-0-b2525b7e1e8e@mobileye.com?part=2