Re: [PATCH] riscv: mm: Implement arch_within_stack_frames() for HARDENED_USERCOPY

[Vivian Wang]

Hi Chen Pei,

Thanks for your patch. I have a few minor suggestions:

On 4/10/26 09:50, cp0613@xxxxxxxxxxxxxxxxx wrote:
> From: Chen Pei <cp0613@xxxxxxxxxxxxxxxxx>
>
> Implement arch_within_stack_frames() to enable precise per-frame stack
> object validation for CONFIG_HARDENED_USERCOPY on RISC-V.
>
> == Background ==
>
> [...]
>
> Signed-off-by: Chen Pei <cp0613@xxxxxxxxxxxxxxxxx>

Patch message is probably too long. You can put additional information
like testing under a --- line to cut those out of the eventual actual
commit message.

Make sure that Signed-off-by is above the cut if you do that.

> ---
>  arch/riscv/Kconfig                   |  1 +
>  arch/riscv/include/asm/thread_info.h | 66 ++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 6fe90591a274..4f765ff608d9 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -150,6 +150,7 @@ config RISCV
>  	select HAVE_ARCH_USERFAULTFD_MINOR if 64BIT && USERFAULTFD
>  	select HAVE_ARCH_USERFAULTFD_WP if 64BIT && MMU && USERFAULTFD && RISCV_ISA_SVRSW60T59B
>  	select HAVE_ARCH_VMAP_STACK if MMU && 64BIT
> +	select HAVE_ARCH_WITHIN_STACK_FRAMES
>  	select HAVE_ASM_MODVERSIONS
>  	select HAVE_BUILDTIME_MCOUNT_SORT
>  	select HAVE_CONTEXT_TRACKING_USER
> diff --git a/arch/riscv/include/asm/thread_info.h b/arch/riscv/include/asm/thread_info.h
> index 36918c9200c9..616a41eb7416 100644
> --- a/arch/riscv/include/asm/thread_info.h
> +++ b/arch/riscv/include/asm/thread_info.h
> @@ -101,6 +101,72 @@ struct thread_info {
>  void arch_release_task_struct(struct task_struct *tsk);
>  int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src);
>  
> +#ifdef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES

Not needed since you've selected it above.

> +/*
> + * RISC-V stack frame layout (with frame pointer enabled):
> + *
> + * high addr
> + *   +------------------+  <--- fp (s0) points here
> + *   |   saved ra       |  fp - 8  (return address)
> + *   |   saved fp       |  fp - 16 (previous frame pointer)

fp - 2*sizeof(void*) probably, to account for RV32?

> + *   +------------------+
> + *   |   local vars     |
> + *   |   arguments      |
> + *   +------------------+  <--- sp
> + * low addr
> + *
> + * The struct stackframe { fp, ra } lives at (fp - sizeof(stackframe)),
> + * i.e. fp[-2]=saved_fp and fp[-1]=saved_ra.
> + *

Maybe link to the authoritative source:

  https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-cc.adoc#frame-pointer-convention

> + * For usercopy safety, we allow copies within [prev_fp, fp - 2*sizeof(void*))
> + * for each frame in the chain, where prev_fp is the fp of the previous
> + * (lower) frame.  This covers local variables and arguments but excludes
> + * the saved ra/fp slots at the top of the frame.
> + *
> + * We walk the frame chain starting from __builtin_frame_address(0) (the
> + * current frame), with prev_fp initialized to current_stack_pointer.
> + * Using current_stack_pointer -- rather than the 'stack' argument (which is
> + * the thread's entire stack base) -- ensures that objects in already-returned
> + * frames (address below current sp) are correctly detected as BAD_STACK,
> + * because no live frame in the chain will claim that region.
> + */
> +__no_kmsan_checks
> +static inline int arch_within_stack_frames(const void * const stack,
> +					   const void * const stackend,
> +					   const void *obj, unsigned long len)
> +{
> +#if defined(CONFIG_FRAME_POINTER)
> +	const void *fp = (const void *)__builtin_frame_address(0);
> +	const void *prev_fp = (const void *)current_stack_pointer;
> +
> +	/*
> +	 * Walk the frame chain. Each iteration checks whether [obj, obj+len)
> +	 * falls within the local-variable area of the current frame:
> +	 *
> +	 *   [prev_fp, fp - 2*sizeof(void*))
> +	 *
> +	 * i.e. from the base of this frame (sp of this frame, which equals
> +	 * the fp of the frame below) up to (but not including) the saved
> +	 * fp/ra area at the top of this frame.
> +	 */
> +	while (stack <= fp && fp < stackend) {

Since we access fp - 2 * sizeof(void *) below, this probably should be
something like this:

    while (stack + 2 * sizeof(void *) <= fp && fp < stackend) {

(Possibly with some casts added.)

> +		const void *frame_vars_end = (const char *)fp - 2 * sizeof(void *);
> +
> +		if (obj + len <= frame_vars_end) {
> +			if (obj >= prev_fp)
> +				return GOOD_FRAME;
> +			return BAD_STACK;
> +		}
> +		prev_fp = fp;
> +		fp = *(const void * const *)((const char *)fp - 2 * sizeof(void *));

Maybe: fp = *(const void * const *)frame_vars_end;

> +	}
> +	return BAD_STACK;
> +#else
> +	return NOT_STACK;
> +#endif
> +}
> +#endif /* CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES */
> +
>  #endif /* !__ASSEMBLER__ */
>  
>  /*

Thanks,
Vivian "dramforever" Wang