| Message ID | 20220224060503.1856302-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | xtensa: Implement "current_stack_pointer" | expand |
On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <keescook@chromium.org> wrote: > > To follow the existing per-arch conventions replace open-coded uses > of asm "sp" as "current_stack_pointer". This will let it be used in > non-arch places (like HARDENED_USERCOPY). > > Cc: Chris Zankel <chris@zankel.net> > Cc: Max Filippov <jcmvbkbc@gmail.com> > Cc: Marc Zyngier <maz@kernel.org> > Cc: linux-xtensa@linux-xtensa.org > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > arch/xtensa/Kconfig | 1 + > arch/xtensa/include/asm/current.h | 2 ++ > arch/xtensa/include/asm/stacktrace.h | 2 +- > arch/xtensa/kernel/irq.c | 3 +-- > 4 files changed, 5 insertions(+), 3 deletions(-) Acked-by: Max Filippov <jcmvbkbc@gmail.com>
On Wed, Feb 23, 2022 at 10:22:59PM -0800, Max Filippov wrote: > On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <keescook@chromium.org> wrote: > > > > To follow the existing per-arch conventions replace open-coded uses > > of asm "sp" as "current_stack_pointer". This will let it be used in > > non-arch places (like HARDENED_USERCOPY). > > > > Cc: Chris Zankel <chris@zankel.net> > > Cc: Max Filippov <jcmvbkbc@gmail.com> > > Cc: Marc Zyngier <maz@kernel.org> > > Cc: linux-xtensa@linux-xtensa.org > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > arch/xtensa/Kconfig | 1 + > > arch/xtensa/include/asm/current.h | 2 ++ > > arch/xtensa/include/asm/stacktrace.h | 2 +- > > arch/xtensa/kernel/irq.c | 3 +-- > > 4 files changed, 5 insertions(+), 3 deletions(-) > > Acked-by: Max Filippov <jcmvbkbc@gmail.com> Thanks! And apologies, my cross-compiler tricked me into thinking this patch compiled without problems. It did, however. I've change the patch slightly to deal with the needed casts: diff --git a/arch/xtensa/include/asm/stacktrace.h b/arch/xtensa/include/asm/stacktrace.h index fe06e8ed162b..a85e785a6288 100644 --- a/arch/xtensa/include/asm/stacktrace.h +++ b/arch/xtensa/include/asm/stacktrace.h @@ -19,14 +19,14 @@ struct stackframe { static __always_inline unsigned long *stack_pointer(struct task_struct *task) { - unsigned long *sp; + unsigned long sp; if (!task || task == current) - __asm__ __volatile__ ("mov %0, a1\n" : "=a"(sp)); + sp = current_stack_pointer; else - sp = (unsigned long *)task->thread.sp; + sp = task->thread.sp; - return sp; + return (unsigned long *)sp; } void walk_stackframe(unsigned long *sp, Shall I send a v2, or just carry this fix in my tree? Sorry for the glitch! -Kees
On Wed, Feb 23, 2022 at 10:43 PM Kees Cook <keescook@chromium.org> wrote: > > On Wed, Feb 23, 2022 at 10:22:59PM -0800, Max Filippov wrote: > > On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > To follow the existing per-arch conventions replace open-coded uses > > > of asm "sp" as "current_stack_pointer". This will let it be used in > > > non-arch places (like HARDENED_USERCOPY). > > > > > > Cc: Chris Zankel <chris@zankel.net> > > > Cc: Max Filippov <jcmvbkbc@gmail.com> > > > Cc: Marc Zyngier <maz@kernel.org> > > > Cc: linux-xtensa@linux-xtensa.org > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > arch/xtensa/Kconfig | 1 + > > > arch/xtensa/include/asm/current.h | 2 ++ > > > arch/xtensa/include/asm/stacktrace.h | 2 +- > > > arch/xtensa/kernel/irq.c | 3 +-- > > > 4 files changed, 5 insertions(+), 3 deletions(-) > > > > Acked-by: Max Filippov <jcmvbkbc@gmail.com> > > Thanks! And apologies, my cross-compiler tricked me into thinking this > patch compiled without problems. It did, however. I've change the patch > slightly to deal with the needed casts: > > diff --git a/arch/xtensa/include/asm/stacktrace.h b/arch/xtensa/include/asm/stacktrace.h > index fe06e8ed162b..a85e785a6288 100644 > --- a/arch/xtensa/include/asm/stacktrace.h > +++ b/arch/xtensa/include/asm/stacktrace.h > @@ -19,14 +19,14 @@ struct stackframe { > > static __always_inline unsigned long *stack_pointer(struct task_struct *task) > { > - unsigned long *sp; > + unsigned long sp; > > if (!task || task == current) > - __asm__ __volatile__ ("mov %0, a1\n" : "=a"(sp)); > + sp = current_stack_pointer; > else > - sp = (unsigned long *)task->thread.sp; > + sp = task->thread.sp; > > - return sp; > + return (unsigned long *)sp; > } > > void walk_stackframe(unsigned long *sp, > > Shall I send a v2, or just carry this fix in my tree? This additional change looks good to me, if you could fold it into the original patch that'd be perfect. But separate patch would also work.
On Wed, Feb 23, 2022 at 10:58:00PM -0800, Max Filippov wrote: > On Wed, Feb 23, 2022 at 10:43 PM Kees Cook <keescook@chromium.org> wrote: > > > > On Wed, Feb 23, 2022 at 10:22:59PM -0800, Max Filippov wrote: > > > On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <keescook@chromium.org> wrote: > > > > > > > > To follow the existing per-arch conventions replace open-coded uses > > > > of asm "sp" as "current_stack_pointer". This will let it be used in > > > > non-arch places (like HARDENED_USERCOPY). > > > > > > > > Cc: Chris Zankel <chris@zankel.net> > > > > Cc: Max Filippov <jcmvbkbc@gmail.com> > > > > Cc: Marc Zyngier <maz@kernel.org> > > > > Cc: linux-xtensa@linux-xtensa.org > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > > --- > > > > arch/xtensa/Kconfig | 1 + > > > > arch/xtensa/include/asm/current.h | 2 ++ > > > > arch/xtensa/include/asm/stacktrace.h | 2 +- > > > > arch/xtensa/kernel/irq.c | 3 +-- > > > > 4 files changed, 5 insertions(+), 3 deletions(-) > > > > > > Acked-by: Max Filippov <jcmvbkbc@gmail.com> > > > > Thanks! And apologies, my cross-compiler tricked me into thinking this > > patch compiled without problems. It did, however. I've change the patch > > slightly to deal with the needed casts: > > > > diff --git a/arch/xtensa/include/asm/stacktrace.h b/arch/xtensa/include/asm/stacktrace.h > > index fe06e8ed162b..a85e785a6288 100644 > > --- a/arch/xtensa/include/asm/stacktrace.h > > +++ b/arch/xtensa/include/asm/stacktrace.h > > @@ -19,14 +19,14 @@ struct stackframe { > > > > static __always_inline unsigned long *stack_pointer(struct task_struct *task) > > { > > - unsigned long *sp; > > + unsigned long sp; > > > > if (!task || task == current) > > - __asm__ __volatile__ ("mov %0, a1\n" : "=a"(sp)); > > + sp = current_stack_pointer; > > else > > - sp = (unsigned long *)task->thread.sp; > > + sp = task->thread.sp; > > > > - return sp; > > + return (unsigned long *)sp; > > } > > > > void walk_stackframe(unsigned long *sp, > > > > Shall I send a v2, or just carry this fix in my tree? > > This additional change looks good to me, if you could > fold it into the original patch that'd be perfect. But separate > patch would also work. Thanks!
diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig index 8ac599aa6d99..887432327613 100644 --- a/arch/xtensa/Kconfig +++ b/arch/xtensa/Kconfig @@ -3,6 +3,7 @@ config XTENSA def_bool y select ARCH_32BIT_OFF_T select ARCH_HAS_BINFMT_FLAT if !MMU + select ARCH_HAS_CURRENT_STACK_POINTER select ARCH_HAS_DMA_PREP_COHERENT if MMU select ARCH_HAS_SYNC_DMA_FOR_CPU if MMU select ARCH_HAS_SYNC_DMA_FOR_DEVICE if MMU diff --git a/arch/xtensa/include/asm/current.h b/arch/xtensa/include/asm/current.h index 5d98a7ad4251..08010dbf5e09 100644 --- a/arch/xtensa/include/asm/current.h +++ b/arch/xtensa/include/asm/current.h @@ -26,6 +26,8 @@ static inline struct task_struct *get_current(void) #define current get_current() +register unsigned long current_stack_pointer __asm__("a1"); + #else #define GET_CURRENT(reg,sp) \ diff --git a/arch/xtensa/include/asm/stacktrace.h b/arch/xtensa/include/asm/stacktrace.h index fe06e8ed162b..4d84fd6bd43c 100644 --- a/arch/xtensa/include/asm/stacktrace.h +++ b/arch/xtensa/include/asm/stacktrace.h @@ -22,7 +22,7 @@ static __always_inline unsigned long *stack_pointer(struct task_struct *task) unsigned long *sp; if (!task || task == current) - __asm__ __volatile__ ("mov %0, a1\n" : "=a"(sp)); + sp = current_stack_pointer; else sp = (unsigned long *)task->thread.sp; diff --git a/arch/xtensa/kernel/irq.c b/arch/xtensa/kernel/irq.c index 15051a8a1539..529fe9245821 100644 --- a/arch/xtensa/kernel/irq.c +++ b/arch/xtensa/kernel/irq.c @@ -36,9 +36,8 @@ asmlinkage void do_IRQ(int hwirq, struct pt_regs *regs) #ifdef CONFIG_DEBUG_STACKOVERFLOW /* Debugging check for stack overflow: is there less than 1KB free? */ { - unsigned long sp; + unsigned long sp = current_stack_pointer; - __asm__ __volatile__ ("mov %0, a1\n" : "=a" (sp)); sp &= THREAD_SIZE - 1; if (unlikely(sp < (sizeof(thread_info) + 1024)))
To follow the existing per-arch conventions replace open-coded uses of asm "sp" as "current_stack_pointer". This will let it be used in non-arch places (like HARDENED_USERCOPY). Cc: Chris Zankel <chris@zankel.net> Cc: Max Filippov <jcmvbkbc@gmail.com> Cc: Marc Zyngier <maz@kernel.org> Cc: linux-xtensa@linux-xtensa.org Signed-off-by: Kees Cook <keescook@chromium.org> --- arch/xtensa/Kconfig | 1 + arch/xtensa/include/asm/current.h | 2 ++ arch/xtensa/include/asm/stacktrace.h | 2 +- arch/xtensa/kernel/irq.c | 3 +-- 4 files changed, 5 insertions(+), 3 deletions(-)