Messages in this thread | | | Date | Wed, 30 Mar 2022 21:59:37 -0700 | From | Kees Cook <> | Subject | Re: [PATCH] riscv: Rename "sp_in_global" to "current_stack_pointer" |
| |
On Wed, Mar 30, 2022 at 04:12:18PM -0700, Palmer Dabbelt wrote: > On Tue, 22 Mar 2022 19:16:28 PDT (-0700), Palmer Dabbelt wrote: > > On Wed, 23 Feb 2022 22:04:11 PST (-0800), keescook@chromium.org wrote: > >> To follow the existing per-arch conventions, rename "sp_in_global" to > >> "current_stack_pointer". This will let it be used in non-arch places > >> (like HARDENED_USERCOPY). > >> > >> Cc: Paul Walmsley <paul.walmsley@sifive.com> > >> Cc: Palmer Dabbelt <palmer@dabbelt.com> > >> Cc: Albert Ou <aou@eecs.berkeley.edu> > >> Cc: Peter Zijlstra <peterz@infradead.org> > >> Cc: Jisheng Zhang <jszhang@kernel.org> > >> Cc: Geert Uytterhoeven <geert@linux-m68k.org> > >> Cc: Mark Rutland <mark.rutland@arm.com> > >> Cc: Chen Huang <chenhuang5@huawei.com> > >> Cc: linux-riscv@lists.infradead.org > >> Signed-off-by: Kees Cook <keescook@chromium.org> > >> --- > >> arch/riscv/Kconfig | 1 + > >> arch/riscv/include/asm/current.h | 2 ++ > >> arch/riscv/kernel/stacktrace.c | 4 +--- > >> 3 files changed, 4 insertions(+), 3 deletions(-) > >> > >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > >> index 5adcbd9b5e88..b120c32697af 100644 > >> --- a/arch/riscv/Kconfig > >> +++ b/arch/riscv/Kconfig > >> @@ -16,6 +16,7 @@ config RISCV > >> select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION > >> select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2 > >> select ARCH_HAS_BINFMT_FLAT > >> + select ARCH_HAS_CURRENT_STACK_POINTER > >> select ARCH_HAS_DEBUG_VM_PGTABLE > >> select ARCH_HAS_DEBUG_VIRTUAL if MMU > >> select ARCH_HAS_DEBUG_WX > >> diff --git a/arch/riscv/include/asm/current.h b/arch/riscv/include/asm/current.h > >> index 1de233d8e8de..21774d868c65 100644 > >> --- a/arch/riscv/include/asm/current.h > >> +++ b/arch/riscv/include/asm/current.h > >> @@ -33,6 +33,8 @@ static __always_inline struct task_struct *get_current(void) > >> > >> #define current get_current() > >> > >> +register unsigned long current_stack_pointer __asm__("sp"); > >> + > >> #endif /* __ASSEMBLY__ */ > >> > >> #endif /* _ASM_RISCV_CURRENT_H */ > >> diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c > >> index 201ee206fb57..278380e2e956 100644 > >> --- a/arch/riscv/kernel/stacktrace.c > >> +++ b/arch/riscv/kernel/stacktrace.c > >> @@ -14,8 +14,6 @@ > >> > >> #include <asm/stacktrace.h> > >> > >> -register unsigned long sp_in_global __asm__("sp"); > >> - > >> #ifdef CONFIG_FRAME_POINTER > >> > >> void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs, > >> @@ -77,7 +75,7 @@ void notrace walk_stackframe(struct task_struct *task, > >> sp = user_stack_pointer(regs); > >> pc = instruction_pointer(regs); > >> } else if (task == NULL || task == current) { > >> - sp = sp_in_global; > >> + sp = current_stack_pointer; > >> pc = (unsigned long)walk_stackframe; > >> } else { > >> /* task blocked in __switch_to */ > > > > Sorry to be slow on this one. This looks good, but another use of > > sp_in_global snuck into fixes and I don't merge fixes into for-next so > > that makes this a bit of a headache. > > > > There's a bunch of stuff in flight, so I'm going to hold off on this > > until after my first week's merge window PR. I intend on merging the > > following, which is at palmer/riscv-sp for now (I'll probably just > > cherry-pick that, I usually start basing stuff on Linus' merge of my PR > > and just cherry-pick in single patches). > > > > commit 7c4f6bb6fb352cfa6028191c2888356b9157a332 > > gpg: Signature made Tue 22 Mar 2022 11:12:19 AM PDT > > gpg: using RSA key 2B3C3747446843B24A943A7A2E1319F35FBB1889 > > gpg: issuer "palmer@dabbelt.com" > > gpg: Good signature from "Palmer Dabbelt <palmer@dabbelt.com>" [ultimate] > > gpg: aka "Palmer Dabbelt <palmer@rivosinc.com>" [ultimate] > > Author: Kees Cook <keescook@chromium.org> > > Date: Wed Feb 23 22:04:11 2022 -0800 > > > > riscv: Rename "sp_in_global" to "current_stack_pointer" > > > > To follow the existing per-arch conventions, rename "sp_in_global" to > > "current_stack_pointer". This will let it be used in non-arch places > > (like HARDENED_USERCOPY). > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > > > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > > index 5adcbd9b5e88..b120c32697af 100644 > > --- a/arch/riscv/Kconfig > > +++ b/arch/riscv/Kconfig > > @@ -16,6 +16,7 @@ config RISCV > > select ARCH_ENABLE_HUGEPAGE_MIGRATION if HUGETLB_PAGE && MIGRATION > > select ARCH_ENABLE_SPLIT_PMD_PTLOCK if PGTABLE_LEVELS > 2 > > select ARCH_HAS_BINFMT_FLAT > > + select ARCH_HAS_CURRENT_STACK_POINTER > > select ARCH_HAS_DEBUG_VM_PGTABLE > > select ARCH_HAS_DEBUG_VIRTUAL if MMU > > select ARCH_HAS_DEBUG_WX > > diff --git a/arch/riscv/include/asm/current.h b/arch/riscv/include/asm/current.h > > index 1de233d8e8de..21774d868c65 100644 > > --- a/arch/riscv/include/asm/current.h > > +++ b/arch/riscv/include/asm/current.h > > @@ -33,6 +33,8 @@ static __always_inline struct task_struct *get_current(void) > > > > #define current get_current() > > > > +register unsigned long current_stack_pointer __asm__("sp"); > > + > > #endif /* __ASSEMBLY__ */ > > > > #endif /* _ASM_RISCV_CURRENT_H */ > > diff --git a/arch/riscv/kernel/stacktrace.c b/arch/riscv/kernel/stacktrace.c > > index 14d2b53ec322..08d11a53f39e 100644 > > --- a/arch/riscv/kernel/stacktrace.c > > +++ b/arch/riscv/kernel/stacktrace.c > > @@ -14,8 +14,6 @@ > > > > #include <asm/stacktrace.h> > > > > -register unsigned long sp_in_global __asm__("sp"); > > - > > #ifdef CONFIG_FRAME_POINTER > > > > void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs, > > @@ -30,7 +28,7 @@ void notrace walk_stackframe(struct task_struct *task, struct pt_regs *regs, > > pc = instruction_pointer(regs); > > } else if (task == NULL || task == current) { > > fp = (unsigned long)__builtin_frame_address(0); > > - sp = sp_in_global; > > + sp = current_stack_pointer; > > pc = (unsigned long)walk_stackframe; > > } else { > > /* task blocked in __switch_to */ > > @@ -78,7 +76,7 @@ void notrace walk_stackframe(struct task_struct *task, > > sp = user_stack_pointer(regs); > > pc = instruction_pointer(regs); > > } else if (task == NULL || task == current) { > > - sp = sp_in_global; > > + sp = current_stack_pointer; > > pc = (unsigned long)walk_stackframe; > > } else { > > /* task blocked in __switch_to */ > > > > Thanks! > > Thanks, this is on for-next. I put HARDENED_USERCOPY=y test in for SLAB > and SLUB, not sure if there's anything else that's interesting to test > (we're missing some bits needed for STACKLEAK).
Excellent; thank you!
-- Kees Cook
| |