[PATCH v2] powerpc: Implement arch_within_stack_frames

[PATCH v2] powerpc: Implement arch_within_stack_frames

[Nicholas Miehlbradt]

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.

Substatially similar to the x86 implementation except using the back
chain to traverse the stack and identify stack frame boundaries.

Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
---
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/
---
 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 COMPAT
diff --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
+
+/*
+ * 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;
+	}
+
+	return BAD_STACK;
+}
+
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* __KERNEL__ */
-- 
2.34.1

Re: [PATCH v2] powerpc: Implement arch_within_stack_frames

[Michael Ellerman]

Nicholas Miehlbradt <nicholas@linux.ibm.com> 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
        ^
        n
> chain 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?

> Signed-off-by: Nicholas Miehlbradt <nicholas@linux.ibm.com>
> ---
> 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/
> ---
>  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 COMPAT
> diff --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