Re: [PATCH v2] powerpc: Implement arch_within_stack_frames
From: Michael Ellerman <mpe@ellerman.id.au>
Date: 2023-01-31 08:05:11
Nicholas Miehlbradt [off-list ref] writes:
Walks the stack when copy_{to,from}_user address is in the stack to
ensure that the object being copied is entirely within a single stack
frame.... and that it exists above the parameter save area, so is not pointing at any stack metadata right?
Substatially similar to the x86 implementation except using the back
^
nchain to traverse the stack and identify stack frame boundaries.
The x86 version does use the back chain (frame pointer) doesn't it? Possibly this comment is just out of date now?
quoted hunk ↗ jump to hunk
Signed-off-by: Nicholas Miehlbradt <redacted> --- v2: Rename PARAMETER_SAVE_OFFSET to STACK_FRAME_PARAMS Add definitions of STACK_FRAME_PARAMS for PPC32 and remove dependancy on PPC64 Ignore the current stack frame and start with it's parent, similar to x86 v1: https://lore.kernel.org/linuxppc-dev/20221214044252.1910657-1-nicholas@linux.ibm.com/ (local) --- arch/powerpc/Kconfig | 1 + arch/powerpc/include/asm/thread_info.h | 36 ++++++++++++++++++++++++++ 2 files changed, 37 insertions(+)diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig index 2ca5418457ed..97ca54773521 100644 --- a/arch/powerpc/Kconfig +++ b/arch/powerpc/Kconfig@@ -198,6 +198,7 @@ config PPC select HAVE_ARCH_KASAN_VMALLOC if HAVE_ARCH_KASAN select HAVE_ARCH_KFENCE if ARCH_SUPPORTS_DEBUG_PAGEALLOC select HAVE_ARCH_RANDOMIZE_KSTACK_OFFSET + select HAVE_ARCH_WITHIN_STACK_FRAMES select HAVE_ARCH_KGDB select HAVE_ARCH_MMAP_RND_BITS select HAVE_ARCH_MMAP_RND_COMPAT_BITS if COMPATdiff --git a/arch/powerpc/include/asm/thread_info.h b/arch/powerpc/include/asm/thread_info.h index af58f1ed3952..c5dce5f239c1 100644 --- a/arch/powerpc/include/asm/thread_info.h +++ b/arch/powerpc/include/asm/thread_info.h@@ -186,6 +186,42 @@ static inline bool test_thread_local_flags(unsigned int flags) #define is_elf2_task() (0) #endif +#if defined(CONFIG_PPC64_ELF_ABI_V1) +#define STACK_FRAME_PARAMS 48 +#elif defined(CONFIG_PPC64_ELF_ABI_V2) +#define STACK_FRAME_PARAMS 32 +#elif defined(CONFIG_PPC32) +#define STACK_FRAME_PARAMS 8 +#endif
Can you please put those in ppc_asm.h? There's an ifdef starting around line 187 where they should fit nicely, it has the __STK_PARAM macros already. The ppc32 case is at line 245. In a subsequent patch we could make the __STK_PARAM macros use your new #defines for the offsets.
+
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ * GOOD_FRAME if within a frame
+ * BAD_STACK if placed across a frame boundary (or outside stack)
+ */
+static inline int arch_within_stack_frames(const void * const stack,
+ const void * const stackend,
+ const void *obj, unsigned long len)
+{
+ const void *params;
+ const void *frame;
+
+ params = *(const void * const *)current_stack_pointer + STACK_FRAME_PARAMS;
+ frame = **(const void * const * const *)current_stack_pointer;
+
+ while (stack <= frame && frame < stackend) {
+ if (obj + len <= frame)
+ return obj >= params ? GOOD_FRAME : BAD_STACK;
+ params = frame + STACK_FRAME_PARAMS;
+ frame = *(const void * const *)frame;
+ }I think the logic here is OK, but the variable naming makes it a bit hard to follow. Normally the stack pointer points at the lowest address of a frame, so the "params" of that frame are at a higher address. But here we have "frame" pointing at the caller frame (higher address) as we check that obj sits above the params of the callee frame (lower address). So "params" and "frame" are different frames. I can't immediately come up with a naming that makes it clearer though. I think it could also be helped with a comment using some ASCII art :) cheers