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 <[email protected]>
Cc: Max Filippov <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
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(-)
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)))
--
2.30.2
On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <[email protected]> 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 <[email protected]>
> Cc: Max Filippov <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: [email protected]
> Signed-off-by: Kees Cook <[email protected]>
> ---
> 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 <[email protected]>
--
Thanks.
-- Max
On Wed, Feb 23, 2022 at 10:43 PM Kees Cook <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > Cc: Max Filippov <[email protected]>
> > > Cc: Marc Zyngier <[email protected]>
> > > Cc: [email protected]
> > > Signed-off-by: Kees Cook <[email protected]>
> > > ---
> > > 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 <[email protected]>
>
> 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.
-- Max
On Wed, Feb 23, 2022 at 10:22:59PM -0800, Max Filippov wrote:
> On Wed, Feb 23, 2022 at 10:05 PM Kees Cook <[email protected]> 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 <[email protected]>
> > Cc: Max Filippov <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > 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 <[email protected]>
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
--
Kees Cook
On Wed, Feb 23, 2022 at 10:58:00PM -0800, Max Filippov wrote:
> On Wed, Feb 23, 2022 at 10:43 PM Kees Cook <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Cc: Max Filippov <[email protected]>
> > > > Cc: Marc Zyngier <[email protected]>
> > > > Cc: [email protected]
> > > > Signed-off-by: Kees Cook <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> >
> > 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!
--
Kees Cook