Re: [PATCH v4 2/3] 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:
- [Medium] riscv: probes: unbounded symbol scan and 0x0000 body match in
  LR/SC check
- [Medium] riscv: probes: uprobe_in_lrsc_body() accepts probe when first
  page read fails
--

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

riscv: probes: reject probes inside an LR/SC sequence

This commit rejects kprobes and uprobes placed inside a constrained LR/SC
atomic sequence by scanning forward up to 16 allowed body instructions for
a matching SC instruction.

> diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
> index 433d9035b8882..78a6f2ac6808c 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;

[ ... ]

> diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
> index 63ee5066def5c..b8df722ac19f7 100644
> --- a/arch/riscv/kernel/probes/kprobes.c
> +++ b/arch/riscv/kernel/probes/kprobes.c
> @@ -49,35 +49,49 @@ 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)
> +/*
> + * 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 int __kprobes arch_check_kprobe(unsigned long addr)
>  {
>  	unsigned long tmp, offset;
>  
>  	/* start iterating at the closest preceding symbol */
>  	if (!kallsyms_lookup_size_offset(addr, NULL, &offset))
> -		return false;
> +		return -EILSEQ;
>  
>  	tmp = addr - offset;
>  
>  	while (tmp <= addr) {
>  		if (tmp == addr)
> -			return true;
> +			if (riscv_probe_in_lrsc_body((u16 *)addr, RISCV_LRSC_SCAN_BYTES))
> +				return -EINVAL;
> +			else
> +				return 0;

[Severity: Medium]
Can arch_check_kprobe() scan past the end of the enclosing symbol and into
an unmapped page?

The commit message states that kprobes scan up to the end of the enclosing
symbol, but arch_check_kprobe() passes NULL for symbolsize to
kallsyms_lookup_size_offset() and always scans RISCV_LRSC_SCAN_BYTES (72
bytes) from addr. In addition, the RVC_OPCODE_C0 check in
riscv_insn_is_lrsc_body() only tests funct3 == 0 without requiring a
non-zero immediate in bits 12:5, so 0x0000 (illegal instruction and zero
padding at the end of a module text section) is treated as a valid
c.addi4spn body instruction. Probing near the end of a function can
therefore scan across symbol boundaries or through trailing zero padding
into an unmapped module vmalloc guard page, causing an unhandled kernel
page fault while holding kprobe_mutex and text_mutex.

Clamping the scan length to symbolsize - offset and rejecting nzuimm == 0
for c.addi4spn would prevent scanning past the symbol or across zero
padding.

[ ... ]

> diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c
> index eb177d0ce8ab0..d97a102621665 100644
> --- a/arch/riscv/kernel/probes/uprobes.c
> +++ b/arch/riscv/kernel/probes/uprobes.c
> @@ -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;
>  }

[Severity: Medium]
Does uprobe_in_lrsc_body() accept a probe on an sc instruction or inside an
LR/SC sequence if get_user_page_vma_remote() fails on the first page?

When the first get_user_page_vma_remote() call fails (for example due to a
pending fatal signal or -ENOMEM), the loop breaks with got == 0 and
riscv_probe_in_lrsc_body(buf, 0) returns false without even checking the
probed instruction already copied in auprobe->insn.
arch_uprobe_analyze_insn() then returns 0, so prepare_uprobe() sets
UPROBE_COPY_INSN on the uprobe permanently; subsequent mappings or tasks
skip prepare_uprobe() and install the breakpoint inside the LR/SC sequence,
livelocking the target task when hit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115-probes-in-lrsc-v4-0-48b9b036bd1d@mobileye.com?part=2