* [PATCH V2] Fix end_of_stack() and location of stack canary for archs using STACK_GROWSUP
@ 2014-09-19 14:35 Chuck Ebbert
2014-09-20 6:42 ` [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP tip-bot for Chuck Ebbert
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Ebbert @ 2014-09-19 14:35 UTC (permalink / raw)
To: Ingo Molnar, Andrew Morton; +Cc: Aaron Tomlin, James Hogan, linux-kernel
Aaron Tomlin recently posted patches [1] to enable checking the stack
canary on every task switch. 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.
[1] http://lkml.org/lkml/2014/9/12/293
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>
---
V2: Fix line length, add Tested-by and Acked-bys
diff a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2611,7 +2611,12 @@ static inline void setup_thread_stack(struct task_struct *p, struct task_struct
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
2014-09-19 14:35 [PATCH V2] Fix end_of_stack() and location of stack canary for archs using STACK_GROWSUP Chuck Ebbert
@ 2014-09-20 6:42 ` tip-bot for Chuck Ebbert
2014-09-20 11:58 ` Chuck Ebbert
0 siblings, 1 reply; 6+ messages in thread
From: tip-bot for Chuck Ebbert @ 2014-09-20 6:42 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, cebbert.lkml, james.hogan, atomlin, tglx
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.
+ */
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
2014-09-20 6:42 ` [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP tip-bot for Chuck Ebbert
@ 2014-09-20 11:58 ` Chuck Ebbert
2014-09-20 14:28 ` Ingo Molnar
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Ebbert @ 2014-09-20 11:58 UTC (permalink / raw)
To: mingo; +Cc: linux-kernel, hpa, james.hogan, atomlin, tglx
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
2014-09-20 11:58 ` Chuck Ebbert
@ 2014-09-20 14:28 ` Ingo Molnar
2014-09-20 15:17 ` [PATCH v3] " Chuck Ebbert
0 siblings, 1 reply; 6+ messages in thread
From: Ingo Molnar @ 2014-09-20 14:28 UTC (permalink / raw)
To: Chuck Ebbert; +Cc: linux-kernel, hpa, james.hogan, atomlin, tglx
* Chuck Ebbert <cebbert.lkml@gmail.com> wrote:
> 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.
Okay, I've zapped the commit, please resubmit the patch with the
comment corrected.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
2014-09-20 14:28 ` Ingo Molnar
@ 2014-09-20 15:17 ` Chuck Ebbert
2014-09-20 17:45 ` [tip:sched/urgent] " tip-bot for Chuck Ebbert
0 siblings, 1 reply; 6+ messages in thread
From: Chuck Ebbert @ 2014-09-20 15:17 UTC (permalink / raw)
To: Ingo Molnar; +Cc: linux-kernel, hpa, james.hogan, atomlin, tglx
Aaron Tomlin recently posted patches [1] to enable checking the stack
canary on every task switch. 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.
[1] http://lkml.org/lkml/2014/9/12/293
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>
---
V3: Fix line length, add comment.
diff a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2609,9 +2609,23 @@ static inline void setup_thread_stack(struct task_struct *p, struct task_struct
task_thread_info(p)->task = p;
}
+/*
+ * Return the address of the last usable long on the stack.
+ *
+ * When the stack grows down, this is just above the thread
+ * info struct. Going any lower will corrupt the threadinfo.
+ *
+ * When the stack grows up, this is the highest address.
+ * Beyond that position, we corrupt data on the next page.
+ */
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
2014-09-20 15:17 ` [PATCH v3] " Chuck Ebbert
@ 2014-09-20 17:45 ` tip-bot for Chuck Ebbert
0 siblings, 0 replies; 6+ messages in thread
From: tip-bot for Chuck Ebbert @ 2014-09-20 17:45 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, hpa, mingo, cebbert.lkml, james.hogan, atomlin, tglx
Commit-ID: 6a40281ab5c1ed8ba2253857118a5d400a2d084b
Gitweb: http://git.kernel.org/tip/6a40281ab5c1ed8ba2253857118a5d400a2d084b
Author: Chuck Ebbert <cebbert.lkml@gmail.com>
AuthorDate: Sat, 20 Sep 2014 10:17:51 -0500
Committer: Ingo Molnar <mingo@kernel.org>
CommitDate: Sat, 20 Sep 2014 19:44:04 +0200
sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP
Aaron Tomlin recently posted patches [1] to enable checking the
stack canary on every task switch. 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.
[1] http://lkml.org/lkml/2014/9/12/293
Signed-off-by: Chuck Ebbert <cebbert.lkml@gmail.com>
Link: http://lkml.kernel.org/r/20140920101751.6c5166b6@as
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Tested-by: James Hogan <james.hogan@imgtec.com> [metag]
Acked-by: James Hogan <james.hogan@imgtec.com>
Acked-by: Aaron Tomlin <atomlin@redhat.com>
---
include/linux/sched.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 5c2c885..1f07040 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 last usable long on the stack.
+ *
+ * When the stack grows down, this is just above the thread
+ * info struct. Going any lower will corrupt the threadinfo.
+ *
+ * When the stack grows up, this is the highest address.
+ * Beyond that position, we corrupt data on the next page.
+ */
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-09-20 17:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-09-19 14:35 [PATCH V2] Fix end_of_stack() and location of stack canary for archs using STACK_GROWSUP Chuck Ebbert
2014-09-20 6:42 ` [tip:sched/urgent] sched: Fix end_of_stack() and location of stack canary for architectures using CONFIG_STACK_GROWSUP tip-bot for Chuck Ebbert
2014-09-20 11:58 ` Chuck Ebbert
2014-09-20 14:28 ` Ingo Molnar
2014-09-20 15:17 ` [PATCH v3] " Chuck Ebbert
2014-09-20 17:45 ` [tip:sched/urgent] " tip-bot for Chuck Ebbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).