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

[sashiko-bot@kernel.org]

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 <vladimir.kondratiev@mobileye.com>

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.

> diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
> index 433d9035b8882..6a4f55b07d04a 100644
> --- a/arch/riscv/kernel/probes/decode-insn.c
> +++ b/arch/riscv/kernel/probes/decode-insn.c

[ ... ]

> @@ -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.

[ ... ]

> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 9e2afabf94e80..1ade363910c96 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -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);

[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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111-probes-in-lrsc-v1-1-4f99ddea5f68@mobileye.com?part=1