Re: [PATCH v7 09/10] riscv: Kconfig: enable HAVE_RELIABLE_STACKTRACE and HAVE_LIVEPATCH

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] riscv: module: fault and UAF in apply_relocate_add() for klp
  relocs
- [Medium] riscv: stacktrace: ftrace_caller() omits frames in reliable
  unwind
- [Medium] riscv: stacktrace: uninitialized frame read in
  return_to_handler()
- [Medium] riscv: stacktrace: arch_rethook_trampoline() omits caller
  frame
--

commit 57947c1aaf895b44b13bb74dddecd8c57f82bb66
Author: Wang Han <wanghan@linux.alibaba.com>

riscv: Kconfig: enable HAVE_RELIABLE_STACKTRACE and HAVE_LIVEPATCH

This commit selects HAVE_RELIABLE_STACKTRACE and HAVE_LIVEPATCH on 64-bit
RISC-V configurations with FRAME_POINTER enabled, and sources
kernel/livepatch/Kconfig.

> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index e7891a2a89da6..4512f67bbfd98 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -186,6 +186,7 @@ config RISCV
>  	select HAVE_KRETPROBES
>  	# https://github.com/ClangBuiltLinux/linux/issues/1881
>  	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION if !LD_IS_LLD
> +	select HAVE_LIVEPATCH if FRAME_POINTER && 64BIT

[Severity: High]
Can klp_apply_object_relocs() -> apply_relocate_add() trigger a store page
fault and use-after-free when applying late .klp.rela.<module>.* relocations?

During load_module(), complete_formation() marks module text and rodata
(including .plt, .got, and .got.plt) read-only under CONFIG_STRICT_MODULE_RWX
before klp_enable_patch() or klp_module_coming() runs.  When
apply_relocate_add() is later called on a module with mod->state !=
MODULE_STATE_UNFORMED, the relocation handlers in arch/riscv/kernel/module.c
(such as riscv_insn_rmw()) and module_emit_got_entry() /
module_emit_plt_entry() in arch/riscv/kernel/module-sections.c write directly
to read-only module memory instead of using text patching helpers and
flushing the instruction cache:

arch/riscv/kernel/module.c:riscv_insn_rmw() {
	...
	parcel[0] = cpu_to_le16(insn);
	parcel[1] = cpu_to_le16(insn >> 16);
	return 0;
}

In addition, module_frob_arch_sections() caches pointers into the temporary
info->sechdrs buffer in mod->arch.plt.shdr, mod->arch.got.shdr, and
mod->arch.got_plt.shdr, which free_copy() frees via vfree() at the end of
load_module():

arch/riscv/kernel/module-sections.c:module_frob_arch_sections() {
	...
	for (i = 0; i < ehdr->e_shnum; i++) {
		if (!strcmp(secstrings + sechdrs[i].sh_name, ".plt"))
			mod->arch.plt.shdr = sechdrs + i;
		else if (!strcmp(secstrings + sechdrs[i].sh_name, ".got"))
			mod->arch.got.shdr = sechdrs + i;
		else if (!strcmp(secstrings + sechdrs[i].sh_name, ".got.plt"))
			mod->arch.got_plt.shdr = sechdrs + i;
	}
	...
}

When late livepatch relocations emit GOT or PLT entries,
module_emit_got_entry() and module_emit_plt_entry() dereference these freed
shdr pointers (e.g. got_sec->shdr->sh_addr), and re-applying relocations when
a target module is reloaded increments num_entries past max_entries and hits
BUG_ON().

>  	select HAVE_MOVE_PMD
>  	select HAVE_MOVE_PUD
>  	select HAVE_PAGE_SIZE_4KB
> @@ -195,6 +196,7 @@ config RISCV
>  	select HAVE_PERF_USER_STACK_DUMP
>  	select HAVE_POSIX_CPU_TIMERS_TASK_WORK
>  	select HAVE_REGS_AND_STACK_ACCESS_API
> +	select HAVE_RELIABLE_STACKTRACE if FRAME_POINTER && 64BIT

[Severity: Medium]
Does arch_stack_walk_reliable() silently omit the traced function and its
caller when unwinding a task inside an ftrace callback or direct trampoline?

With CONFIG_DYNAMIC_FTRACE_WITH_ARGS, the patchable entry of a traced
function jumps to ftrace_caller before the function sets up its stack frame.
At that point, t0 holds the traced function's PC, ra holds the return address
in its caller, and s0 still holds the caller's frame pointer.  In
arch/riscv/kernel/mcount-dyn.S, SAVE_ABI_REGS saves t0, x1, and x8 into
struct __arch_ftrace_regs without constructing frame records on the stack or
updating s0:

arch/riscv/kernel/mcount-dyn.S:SAVE_ABI_REGS {
	addi	sp, sp, -FREGS_SIZE_ON_STACK
	REG_S	t0,  FREGS_EPC(sp)
	REG_S	x1,  FREGS_RA(sp)
	REG_S	x8,  FREGS_S0(sp)
	...
}

When ftrace_caller calls op->func, op->func saves the unchanged s0 in its
frame record.  Unwinding that frame record in kunwind_next_frame_record()
advances directly to the caller's caller with state->source ==
KUNWIND_SOURCE_FRAME, which arch_reliable_kunwind_consume_entry() accepts as
reliable.  During a livepatch transition, klp_check_stack() can miss that a
preempted or blocked task is executing inside a patched function or its
caller.

[Severity: Medium]
Can kunwind_next_frame_record() read uninitialized stack memory when
unwinding through return_to_handler()?

In arch/riscv/kernel/mcount.S, SAVE_RET_ABI_STATE allocates
FREGS_SIZE_ON_STACK bytes, saves ra and s0 at low offsets from sp, and sets
s0 to sp + FREGS_SIZE_ON_STACK before return_to_handler calls
ftrace_return_to_handler():

arch/riscv/kernel/mcount.S:SAVE_RET_ABI_STATE {
	addi	sp, sp, -FREGS_SIZE_ON_STACK
	REG_S	ra, FREGS_RA(sp)
	REG_S	s0, FREGS_S0(sp)
	REG_S	a0, FREGS_A0(sp)
	REG_S	a1, FREGS_A1(sp)
	addi	s0, sp, FREGS_SIZE_ON_STACK
}

The 16 bytes at s0 - 16 (sp + FREGS_SIZE_ON_STACK - 16) are never
initialized.  When unwinding a task inside ftrace_return_to_handler(),
kunwind_next_frame_record() restores fp = sp + FREGS_SIZE_ON_STACK and on the
next step reads fp - sizeof(*record) as a struct frame_record, either
triggering WARN_ON_ONCE(tsk == current) in kunwind_next_frame_record_meta()
if those bytes happen to be zero or following a bogus frame pointer and
skipping live frames while returning 0 (reliable).

[Severity: Medium]
Does unwinding from inside arch_rethook_trampoline_callback() omit the caller
of the probed function while still reporting a reliable stack trace?

When a kretprobed function returns to arch_rethook_trampoline(), its epilogue
has already restored s0 to its caller's frame pointer, while the return
address in the caller is held in current->rethooks.  In
arch/riscv/kernel/probes/rethook_trampoline.S, arch_rethook_trampoline
allocates PT_SIZE_ON_STACK without initializing S_STACKFRAME
(FRAME_META_TYPE_PT_REGS) or updating s0 before calling
arch_rethook_trampoline_callback():

arch/riscv/kernel/probes/rethook_trampoline.S:arch_rethook_trampoline() {
	addi sp, sp, -(PT_SIZE_ON_STACK)
	save_all_base_regs

	move a0, sp /* pt_regs */

	call arch_rethook_trampoline_callback
	...
}

When kunwind unwinds arch_rethook_trampoline_callback()'s frame record,
state->common.pc is inside arch_rethook_trampoline (past the entry address
checked by is_kretprobe_trampoline()) and state->common.fp is the caller's
frame pointer.  The next unwind step reads the frame record of the caller's
caller, skipping the caller of the probed function and returning 0
(reliable).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110013750.36800-1-xueshuai@linux.alibaba.com?part=9