linux-kernel - Re: [PATCH fixes] riscv: uprobes: Add missing fence.i after building the XOL buffer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b864fc90-1297-4e1d-8487-61049e23e405@sifive.com>
Date: Fri, 18 Apr 2025 11:18:59 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Björn Töpel <bjorn@...nel.org>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Alexandre Ghiti <alex@...ti.fr>,
 linux-riscv@...ts.infradead.org, Guo Ren <guoren@...nel.org>
Cc: Björn Töpel <bjorn@...osinc.com>,
 Masami Hiramatsu <mhiramat@...nel.org>, Oleg Nesterov <oleg@...hat.com>,
 Peter Zijlstra <peterz@...radead.org>, linux-kernel@...r.kernel.org,
 linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH fixes] riscv: uprobes: Add missing fence.i after building
 the XOL buffer

Hi Björn,

On 2025-04-17 2:49 AM, Björn Töpel wrote:
> From: Björn Töpel <bjorn@...osinc.com>
> 
> The XOL (execute out-of-line) buffer is used to single-step the
> replaced instruction(s) for uprobes. The RISC-V port was missing a
> proper fence.i (i$ flushing) after constructing the XOL buffer, which
> can result in incorrect execution of stale/broken instructions.
> 
> This was found running the BPF selftests "test_progs:
> uprobe_autoattach, attach_probe" on the Spacemit K1/X60, where the
> uprobes tests randomly blew up.
> 
> Fixes: 74784081aac8 ("riscv: Add uprobes supported")
> Signed-off-by: Björn Töpel <bjorn@...osinc.com>
> ---
>  arch/riscv/kernel/probes/uprobes.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/arch/riscv/kernel/probes/uprobes.c b/arch/riscv/kernel/probes/uprobes.c
> index 4b3dc8beaf77..4faef92dd771 100644
> --- a/arch/riscv/kernel/probes/uprobes.c
> +++ b/arch/riscv/kernel/probes/uprobes.c
> @@ -176,13 +176,6 @@ void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
>  		*(uprobe_opcode_t *)dst = __BUG_INSN_32;
>  	}
>  
> +	flush_icache_range((unsigned long)dst, (unsigned long)dst + len);

This works because flush_icache_range currently ignores the range, but
semantically is not quite right, because of the line just above the context that
increments dst. If the range was respected, this would only flush the ebreak,
not the preceding single-stepped instruction.

Regards,
Samuel

>  	kunmap_atomic(kaddr);
> -
> -	/*
> -	 * We probably need flush_icache_user_page() but it needs vma.
> -	 * This should work on most of architectures by default. If
> -	 * architecture needs to do something different it can define
> -	 * its own version of the function.
> -	 */
> -	flush_dcache_page(page);
>  }
> 
> base-commit: 1a1d569a75f3ab2923cb62daf356d102e4df2b86