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

[袁晓峰 <yuanxiaofeng@eswincomputing.com>]

Hi Masami,


I looked into the issue Sashiko reported for riscv_probe_decode_insn().
Sharing the analysis below; the short version is that this read
predates this series, and I'm happy to send a separate patch if the
maintainers think it is worth fixing.


First, the context: "probe_opcode_t insn = *addr;" is pre-existing
code.  This series only adds two RISCV_INSN_REJECTED() entries, which
consume the already-loaded value and do not change how memory is
accessed.


On the unaligned part: with default configurations, misaligned
accesses are emulated, either by the firmware or by the kernel itself
(RISCV_SCALAR_MISALIGNED), so an oops would require a NONPORTABLE
kernel built with RISCV_SLOW/EFFICIENT_UNALIGNED_ACCESS running on
hardware that does not actually support unaligned accesses.


On the page-crossing part: this one is real in theory, though the
window is narrow.  It cannot happen for built-in kernel text (the
linear mapping is contiguous).  For modules, it needs a compressed
instruction at the last two bytes of a module text page followed by
the vmalloc guard page, plus a kprobe registered exactly there.  A
32-bit instruction cannot sit at that location on a working module
(its encoding would cross into the unmapped guard page, so it could
never be fetched), which is also why the two-u16-step copy in
arch_prepare_kprobe() is safe.


If it is worth fixing, the cleanest approach seems to be passing the
opcode by value instead of a pointer: arch_prepare_kprobe() already
holds a safely copied p->opcode, and the uprobes caller passes a stack
variable, so both call sites would be covered.  There is a third
similar raw read in uprobes.c (the auprobe->insn[] dereference) that
could be folded in as well.


Either way, this doesn't need to be handled in the series: the
upcoming v7 will only address Nam's review comments.


Thanks,
Xiaofeng


> -----原始邮件-----
> 发件人: sashiko-bot@kernel.org
> 发送时间:2026-10-16 16:29:42 (星期五)
> 收件人: "Xiaofeng Yuan" <yuanxiaofeng@eswincomputing.com>
> 抄送: linux-trace-kernel@vger.kernel.org
> 主题: Re: [PATCH v6 1/2] riscv: kprobes: reject probes inside LR/SC sequences
> 
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] riscv: kprobes: unsafe 32-bit read in riscv_probe_decode_insn can page fault
> --
> 
> commit 6f940578d2f096970e9abd6cc2894de008e29f6f
> Author: Xiaofeng Yuan <yuanxiaofeng@eswincomputing.com>
> 
>     riscv: kprobes: reject probes inside LR/SC sequences
> 
> This patch rejects kprobes placed inside LR/SC sequences or on the LR/SC
> instructions themselves to avoid livelock. It updates riscv_probe_decode_insn()
> to reject the instructions and adds instruction walking logic in
> arch_check_kprobe() to reject addresses within a sequence.
> 
> [ ... ]
> 
> > diff --git a/arch/riscv/kernel/probes/decode-insn.c b/arch/riscv/kernel/probes/decode-insn.c
> > index 433d9035b8882..e6a8894677b10 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);
> 
> [Severity: High]
> Does evaluating these macros rely on the insn variable, which is read
> unconditionally as a native 32-bit integer earlier in the function?
> 
> arch/riscv/kernel/probes/decode-insn.c:riscv_probe_decode_insn() {
> 	probe_opcode_t insn = *addr;
> 	...
> 
> If addr is 2-byte aligned, will this native 32-bit dereference perform
> an unaligned memory access, potentially oopsing on RISC-V hardware that
> lacks kernel-mode unaligned access support?
> 
> Furthermore, if addr points to a 16-bit compressed instruction at the
> end of a mapped page and the next page is an unmapped vmalloc guard
> page, could this 32-bit read cross the page boundary into unmapped
> memory?
> 
> Would this trigger a kernel page fault and panic when tracing tools
> register a kprobe at such an address?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261016081706.1085-1-yuanxiaofeng@eswincomputing.com?part=1