Re: [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP

[Chuck Ebbert <cebbert.lkml@gmail.com>]

On Fri, 19 Sep 2014 23:42:37 -0700
tip-bot for Chuck Ebbert <tipbot@zytor.com> wrote:

> Commit-ID:  a3215fb47c7ecb814dc16815245db4f375841268
> Gitweb:     http://git.kernel.org/tip/a3215fb47c7ecb814dc16815245db4f375841268
> Author:     Chuck Ebbert <cebbert.lkml@gmail.com>
> AuthorDate: Fri, 19 Sep 2014 09:35:05 -0500
> Committer:  Ingo Molnar <mingo@kernel.org>
> CommitDate: Sat, 20 Sep 2014 08:38:16 +0200
> 
> sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
> 
> Aaron Tomlin recently posted patches to enable checking the
> stack canary on every task switch:
> 
>   http://lkml.org/lkml/2014/9/12/293
> 
> Looking at the canary code, I realized that every arch
> (except ia64, which adds some space for register spill
> above the stack) shares a definition of end_of_stack()
> that makes it the first long after the threadinfo.
> 
> For stacks that grow down, this low address is correct
> because the stack starts at the end of the thread area
> and grows toward lower addresses. However, for stacks
> that grow up, toward higher addresses, this is wrong.
> (The stack actually grows away from the canary.)
> 
> On these archs end_of_stack() should return the address
> of the last long, at the highest possible address for
> the stack.
> 
> Signed-off-by: Chuck Ebbert <cebbert.lkml@gmail.com>
> Tested-by: James Hogan <james.hogan@imgtec.com> [metag]
> Acked-by: James Hogan <james.hogan@imgtec.com>
> Acked-by: Aaron Tomlin <atomlin@redhat.com>
> Link: http://lkml.kernel.org/r/20140919093505.62681e43@as
> [ Added comments to end_of_stack(). ]
> Signed-off-by: Ingo Molnar <mingo@kernel.org>
> ---
>  include/linux/sched.h | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 5c2c885..0e20a24 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2608,9 +2608,22 @@ static inline void setup_thread_stack(struct task_struct *p, struct task_struct
>  	task_thread_info(p)->task = p;
>  }
>  
> +/*
> + * Return the address of the first long before (or after,
> + * depending on the architecture's default stack growth
> + * direction) * a task's task_info structure, which is the
> + * kernel stack's last usable spot.
> + *
> + * This is the point of no return, if the stack grows
> + * beyond that position, we corrupt the task's state.
> + */

This comment you added is not correct for archs where the stack grows
up. The threadinfo is always at the lowest address on the stack in
every case. Instead of corrupting the thread info, a stack overflow
will corrupt whatever is on the next page after the stack if it grows
upward.

>  static inline unsigned long *end_of_stack(struct task_struct *p)
>  {
> +#ifdef CONFIG_STACK_GROWSUP
> +	return (unsigned long *)((unsigned long)task_thread_info(p) + THREAD_SIZE) - 1;
> +#else
>  	return (unsigned long *)(task_thread_info(p) + 1);
> +#endif
>  }
>  
>  #endif