[PATCH] powerpc/64: Implement arch_within_stack_frames

[Nicholas Miehlbradt]

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 <nicholas at 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