| Message ID | 20200416161245.148813-4-samitolvanen@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for Clang's Shadow Call Stack | expand |
On Thu, Apr 16, 2020 at 09:12:36AM -0700, Sami Tolvanen wrote: > Implements CONFIG_DEBUG_STACK_USAGE for shadow stacks. When enabled, > also prints out the highest shadow stack usage per process. > > Signed-off-by: Sami Tolvanen <samitolvanen@google.com> > Reviewed-by: Kees Cook <keescook@chromium.org> > --- > kernel/scs.c | 39 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/kernel/scs.c b/kernel/scs.c > index 5245e992c692..ad74d13f2c0f 100644 > --- a/kernel/scs.c > +++ b/kernel/scs.c > @@ -184,6 +184,44 @@ int scs_prepare(struct task_struct *tsk, int node) > return 0; > } > > +#ifdef CONFIG_DEBUG_STACK_USAGE > +static inline unsigned long scs_used(struct task_struct *tsk) > +{ > + unsigned long *p = __scs_base(tsk); > + unsigned long *end = scs_magic(p); > + unsigned long s = (unsigned long)p; > + > + while (p < end && READ_ONCE_NOCHECK(*p)) > + p++; I think the expectation is that the caller has already checked that the stack is not corrupted, so I'd probably throw a couple of underscores in front of the function name, along with a comment. Also, is tsk ever != current? > + > + return (unsigned long)p - s; > +} > + > +static void scs_check_usage(struct task_struct *tsk) > +{ > + static DEFINE_SPINLOCK(lock); > + static unsigned long highest; > + unsigned long used = scs_used(tsk); > + > + if (used <= highest) > + return; > + > + spin_lock(&lock); > + > + if (used > highest) { > + pr_info("%s (%d): highest shadow stack usage: %lu bytes\n", > + tsk->comm, task_pid_nr(tsk), used); > + highest = used; > + } > + > + spin_unlock(&lock); Do you really need this lock? I'd have thought you could cmpxchg() highest instead. Will
On Mon, Apr 20, 2020 at 06:17:42PM +0100, Will Deacon wrote: > > +#ifdef CONFIG_DEBUG_STACK_USAGE > > +static inline unsigned long scs_used(struct task_struct *tsk) > > +{ > > + unsigned long *p = __scs_base(tsk); > > + unsigned long *end = scs_magic(p); > > + unsigned long s = (unsigned long)p; > > + > > + while (p < end && READ_ONCE_NOCHECK(*p)) > > + p++; > > I think the expectation is that the caller has already checked that the > stack is not corrupted, so I'd probably throw a couple of underscores > in front of the function name, along with a comment. Correct. I'll do that. > Also, is tsk ever != current? This is only called from scs_release(), so tsk is never current. > > +static void scs_check_usage(struct task_struct *tsk) > > +{ > > + static DEFINE_SPINLOCK(lock); > > + static unsigned long highest; > > + unsigned long used = scs_used(tsk); > > + > > + if (used <= highest) > > + return; > > + > > + spin_lock(&lock); > > + > > + if (used > highest) { > > + pr_info("%s (%d): highest shadow stack usage: %lu bytes\n", > > + tsk->comm, task_pid_nr(tsk), used); > > + highest = used; > > + } > > + > > + spin_unlock(&lock); > > Do you really need this lock? I'd have thought you could cmpxchg() > highest instead. This is similar to check_stack_usage in kernel/exit.c, but yes, I can change this to a cmpxchg() loop instead. Sami
diff --git a/kernel/scs.c b/kernel/scs.c index 5245e992c692..ad74d13f2c0f 100644 --- a/kernel/scs.c +++ b/kernel/scs.c @@ -184,6 +184,44 @@ int scs_prepare(struct task_struct *tsk, int node) return 0; } +#ifdef CONFIG_DEBUG_STACK_USAGE +static inline unsigned long scs_used(struct task_struct *tsk) +{ + unsigned long *p = __scs_base(tsk); + unsigned long *end = scs_magic(p); + unsigned long s = (unsigned long)p; + + while (p < end && READ_ONCE_NOCHECK(*p)) + p++; + + return (unsigned long)p - s; +} + +static void scs_check_usage(struct task_struct *tsk) +{ + static DEFINE_SPINLOCK(lock); + static unsigned long highest; + unsigned long used = scs_used(tsk); + + if (used <= highest) + return; + + spin_lock(&lock); + + if (used > highest) { + pr_info("%s (%d): highest shadow stack usage: %lu bytes\n", + tsk->comm, task_pid_nr(tsk), used); + highest = used; + } + + spin_unlock(&lock); +} +#else +static inline void scs_check_usage(struct task_struct *tsk) +{ +} +#endif + bool scs_corrupted(struct task_struct *tsk) { unsigned long *magic = scs_magic(__scs_base(tsk)); @@ -200,6 +238,7 @@ void scs_release(struct task_struct *tsk) return; WARN_ON(scs_corrupted(tsk)); + scs_check_usage(tsk); scs_account(tsk, -1); task_set_scs(tsk, NULL);