On 14/12/2022 10:39 pm, Nicholas Piggin wrote:
On Wed Dec 14, 2022 at 6:39 PM AEST, Christophe Leroy wrote:
Le 14/12/2022 à 05:42, Nicholas Miehlbradt a écrit :
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 <nicho...@linux.ibm.com>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/thread_info.h | 38 ++++++++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 2ca5418457ed..4c59d139ea83 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 if PPC64
Why don't you do something that works for both PPC32 and PPC64 ?
+1
I'm not familiar with the 32bit ABI, but from a quick glance through it
seems like the only thing that would need to change is to set then
PARAMETER_SAVE_OFFSET (to be renamed in the next version as per
suggestions) to 8 bytes, the layout of the stack and the back chain
remains the same. Is there something else that I am missing or is that it?
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..efdf39e07884 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@ -186,6 +186,44 @@ static inline bool test_thread_local_flags(unsigned int
flags)
#define is_elf2_task() (0)
#endif
+#ifdef CONFIG_PPC64
+
+#ifdef CONFIG_PPC64_ELF_ABI_V1
+#define PARAMETER_SAVE_OFFSET 48
+#else
+#define PARAMETER_SAVE_OFFSET 32
+#endif
Why not use STACK_INT_FRAME_REGS, defined in asm/ptrace.h ?
I think use a STACK_FRAME prefixed define in asm/ptrace.h, but maybe
avoid overloading the STACK_INT_ stuff for this.
+
+/*
+ * 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 *frame;
+ const void *oldframe;
+
+ oldframe = (const void *)current_stack_pointer;
+ frame = *(const void * const *)oldframe;
This is not the same as x86, they start with the parent of the current
frame. I assume because the way the caller is set up (with a noinline
function from an out of line call), then there must be at least one
stack frame that does not have to be checked, but if I'm wrong about
that and there is some reason we need to be different it should be
commented..
Yes, this is something that I overlooked, the current frame is created
as a result of the call to copy_{to,from}_user and should therefore not
contain any data being copied.
+
+ while (stack <= frame && frame < stackend) {
+ if (obj + len <= frame)
+ return obj >= oldframe + PARAMETER_SAVE_OFFSET ?
+ GOOD_FRAME : BAD_STACK;
+ oldframe = frame;
+ frame = *(const void * const *)oldframe;
+ }
+
+ return BAD_STACK;
+}
What about:
+ const void *frame;
+ const void *params;
+
+ params = (const void *)current_stack_pointer + STACK_INT_FRAME_REGS;
+ frame = *(const void * const *)current_stack_pointer;
+
+ while (stack <= frame && frame < stackend) {
+ if (obj + len <= frame)
+ return obj >= params ? GOOD_FRAME : BAD_STACK;
+ params = frame + STACK_INT_FRAME_REGS;
+ frame = *(const void * const *)frame;
+ }
+
+ return BAD_STACK;
What about just copying x86's implementation including using
__builtin_frame_address(1/2)? Are those builtins reliable for all
our targets and compiler versions?
From what I found it has undefined behavior. Since x86 has it's use
guarded behind CONFIG_FRAME_POINTER which I couldn't find used in the
ppc code I decided it was best to avoid them. Could be wrong though.
For bonus points, extract the x86 code out into asm-generic and
make it usable by both -
static inline int generic_within_stack_frames(unsigned int ptr_offset,
unsigned int vars_offset,
const void * const stack,
const void * const stackend,
const void *obj, unsigned long
len)
And our arch_within_stack_frames can just be
return generic_within_stack_frames(0, STACK_FRAME_ARGS_OFFSET,
stack, stackend, obj, len);
Thanks,
Nick
