[PATCH 1/2] RISC-V: Align the shadow stack

[PATCH 1/2] RISC-V: Align the shadow stack

[Palmer Dabbelt]

The standard RISC-V ABIs all require 16-byte stack alignment.  We're
only calling that one function on the shadow stack so I doubt it'd
result in a real issue, but might as well keep this lined up.

Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
 arch/riscv/kernel/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index be54ccea8c47..acdfcacd7e57 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
  * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
  * to get per-cpu overflow stack(get_overflow_stack).
  */
-long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
+long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
 asmlinkage unsigned long get_overflow_stack(void)
 {
 	return (unsigned long)this_cpu_ptr(overflow_stack) +
-- 
2.38.1

[PATCH 2/2] RISC-V: Add some comments about the shadow and overflow stacks

[Palmer Dabbelt]

It took me a while to page all this back in when trying to review the
recent spin_shadow_stack, so I figured I'd just write up some comments.

Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
---
 arch/riscv/kernel/traps.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
index acdfcacd7e57..336d4aadadb1 100644
--- a/arch/riscv/kernel/traps.c
+++ b/arch/riscv/kernel/traps.c
@@ -200,18 +200,18 @@ void __init trap_init(void)
 }
 
 #ifdef CONFIG_VMAP_STACK
+/*
+ * Extra stack space that allows us to provide panic messages when the kernel
+ * has overflowed its stack.
+ */
 static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
 		overflow_stack)__aligned(16);
 /*
- * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
- * to get per-cpu overflow stack(get_overflow_stack).
+ * A temporary stack for use by handle_kernel_stack_overflow.  This is used so
+ * we can call into C code to get the per-hart overflow stack.  Usage of this
+ * stack must be protected by spin_shadow_stack.
  */
 long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
-asmlinkage unsigned long get_overflow_stack(void)
-{
-	return (unsigned long)this_cpu_ptr(overflow_stack) +
-		OVERFLOW_STACK_SIZE;
-}
 
 /*
  * A pseudo spinlock to protect the shadow stack from being used by multiple
@@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
  */
 unsigned long spin_shadow_stack;
 
+asmlinkage unsigned long get_overflow_stack(void)
+{
+	return (unsigned long)this_cpu_ptr(overflow_stack) +
+		OVERFLOW_STACK_SIZE;
+}
+
 asmlinkage void handle_bad_stack(struct pt_regs *regs)
 {
 	unsigned long tsk_stk = (unsigned long)current->stack;
-- 
2.38.1

Re: [PATCH 2/2] RISC-V: Add some comments about the shadow and overflow stacks

[Guo Ren]

On Wed, Nov 30, 2022 at 10:35 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> It took me a while to page all this back in when trying to review the
> recent spin_shadow_stack, so I figured I'd just write up some comments.
>
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
>  arch/riscv/kernel/traps.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index acdfcacd7e57..336d4aadadb1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -200,18 +200,18 @@ void __init trap_init(void)
>  }
>
>  #ifdef CONFIG_VMAP_STACK
> +/*
> + * Extra stack space that allows us to provide panic messages when the kernel
> + * has overflowed its stack.
> + */
>  static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>                 overflow_stack)__aligned(16);
>  /*
> - * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
> - * to get per-cpu overflow stack(get_overflow_stack).
> + * A temporary stack for use by handle_kernel_stack_overflow.  This is used so
> + * we can call into C code to get the per-hart overflow stack.  Usage of this
> + * stack must be protected by spin_shadow_stack.
Reviewed-by: Guo Ren <guoren@kernel.org>

>   */
>  long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> -asmlinkage unsigned long get_overflow_stack(void)
> -{
> -       return (unsigned long)this_cpu_ptr(overflow_stack) +
> -               OVERFLOW_STACK_SIZE;
> -}
>
>  /*
>   * A pseudo spinlock to protect the shadow stack from being used by multiple
> @@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
>   */
>  unsigned long spin_shadow_stack;
>
> +asmlinkage unsigned long get_overflow_stack(void)
> +{
> +       return (unsigned long)this_cpu_ptr(overflow_stack) +
> +               OVERFLOW_STACK_SIZE;
> +}
> +
>  asmlinkage void handle_bad_stack(struct pt_regs *regs)
>  {
>         unsigned long tsk_stk = (unsigned long)current->stack;
> --
> 2.38.1
>


-- 
Best Regards
 Guo Ren

Re: [PATCH 2/2] RISC-V: Add some comments about the shadow and overflow stacks

[Jisheng Zhang]

On Tue, Nov 29, 2022 at 06:35:15PM -0800, Palmer Dabbelt wrote:
> It took me a while to page all this back in when trying to review the
> recent spin_shadow_stack, so I figured I'd just write up some comments.
> 
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

Reviewed-by: Jisheng Zhang <jszhang@kernel.org>

> ---
>  arch/riscv/kernel/traps.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index acdfcacd7e57..336d4aadadb1 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -200,18 +200,18 @@ void __init trap_init(void)
>  }
>  
>  #ifdef CONFIG_VMAP_STACK
> +/*
> + * Extra stack space that allows us to provide panic messages when the kernel
> + * has overflowed its stack.
> + */
>  static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>  		overflow_stack)__aligned(16);
>  /*
> - * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
> - * to get per-cpu overflow stack(get_overflow_stack).
> + * A temporary stack for use by handle_kernel_stack_overflow.  This is used so
> + * we can call into C code to get the per-hart overflow stack.  Usage of this
> + * stack must be protected by spin_shadow_stack.
>   */
>  long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> -asmlinkage unsigned long get_overflow_stack(void)
> -{
> -	return (unsigned long)this_cpu_ptr(overflow_stack) +
> -		OVERFLOW_STACK_SIZE;
> -}
>  
>  /*
>   * A pseudo spinlock to protect the shadow stack from being used by multiple
> @@ -222,6 +222,12 @@ asmlinkage unsigned long get_overflow_stack(void)
>   */
>  unsigned long spin_shadow_stack;
>  
> +asmlinkage unsigned long get_overflow_stack(void)
> +{
> +	return (unsigned long)this_cpu_ptr(overflow_stack) +
> +		OVERFLOW_STACK_SIZE;
> +}
> +
>  asmlinkage void handle_bad_stack(struct pt_regs *regs)
>  {
>  	unsigned long tsk_stk = (unsigned long)current->stack;
> -- 
> 2.38.1
> 

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Khem Raj]

Hi Palmer

On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.

Is 16-byte alignment required on rv32 as well ?

>
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> ---
>  arch/riscv/kernel/traps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index be54ccea8c47..acdfcacd7e57 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>   * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
>   * to get per-cpu overflow stack(get_overflow_stack).
>   */
> -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>  asmlinkage unsigned long get_overflow_stack(void)
>  {
>         return (unsigned long)this_cpu_ptr(overflow_stack) +
> --
> 2.38.1
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Palmer Dabbelt]

On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
> Hi Palmer
>
> On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
>> only calling that one function on the shadow stack so I doubt it'd
>> result in a real issue, but might as well keep this lined up.
>
> Is 16-byte alignment required on rv32 as well ?

For the standard ABIs that's the case, it's so the Q extension can spill 
without aligning the stack.  There's also at least a proposed embedded 
ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack 
alignment has an impact on some use cases.

>> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
>> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> ---
>>  arch/riscv/kernel/traps.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> index be54ccea8c47..acdfcacd7e57 100644
>> --- a/arch/riscv/kernel/traps.c
>> +++ b/arch/riscv/kernel/traps.c
>> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>>   * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
>>   * to get per-cpu overflow stack(get_overflow_stack).
>>   */
>> -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
>> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>>  asmlinkage unsigned long get_overflow_stack(void)
>>  {
>>         return (unsigned long)this_cpu_ptr(overflow_stack) +
>> --
>> 2.38.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Khem Raj]

On Tue, Nov 29, 2022 at 6:50 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
> > Hi Palmer
> >
> > On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
> >>
> >> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
> >> only calling that one function on the shadow stack so I doubt it'd
> >> result in a real issue, but might as well keep this lined up.
> >
> > Is 16-byte alignment required on rv32 as well ?
>
> For the standard ABIs that's the case, it's so the Q extension can spill
> without aligning the stack.  There's also at least a proposed embedded
> ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack
> alignment has an impact on some use cases.

Thanks, so in this case 16byte will be valid for both rv64/rv32 here.

>
> >> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> >> ---
> >>  arch/riscv/kernel/traps.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> >> index be54ccea8c47..acdfcacd7e57 100644
> >> --- a/arch/riscv/kernel/traps.c
> >> +++ b/arch/riscv/kernel/traps.c
> >> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
> >>   * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
> >>   * to get per-cpu overflow stack(get_overflow_stack).
> >>   */
> >> -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
> >> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
> >>  asmlinkage unsigned long get_overflow_stack(void)
> >>  {
> >>         return (unsigned long)this_cpu_ptr(overflow_stack) +
> >> --
> >> 2.38.1
> >>
> >>
> >> _______________________________________________
> >> linux-riscv mailing list
> >> linux-riscv@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Palmer Dabbelt]

On Tue, 29 Nov 2022 18:56:48 PST (-0800), Khem Raj wrote:
> On Tue, Nov 29, 2022 at 6:50 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> On Tue, 29 Nov 2022 18:47:55 PST (-0800), Khem Raj wrote:
>> > Hi Palmer
>> >
>> > On Tue, Nov 29, 2022 at 6:36 PM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>> >>
>> >> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
>> >> only calling that one function on the shadow stack so I doubt it'd
>> >> result in a real issue, but might as well keep this lined up.
>> >
>> > Is 16-byte alignment required on rv32 as well ?
>>
>> For the standard ABIs that's the case, it's so the Q extension can spill
>> without aligning the stack.  There's also at least a proposed embedded
>> ABI that has just XLEN (32-bit on rv32) alignment, as the bigger stack
>> alignment has an impact on some use cases.
>
> Thanks, so in this case 16byte will be valid for both rv64/rv32 here.

Yes, though the long-alignment wouldn't break anything because we don't 
have Q support and we're just calling that one function -- it's not like 
the compiler is actively checking for 16-byte alignment or anything, 
it's just assuming it.

Still best to keep things to the spec where we can, though.

>> >> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
>> >> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
>> >> ---
>> >>  arch/riscv/kernel/traps.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
>> >> index be54ccea8c47..acdfcacd7e57 100644
>> >> --- a/arch/riscv/kernel/traps.c
>> >> +++ b/arch/riscv/kernel/traps.c
>> >> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>> >>   * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
>> >>   * to get per-cpu overflow stack(get_overflow_stack).
>> >>   */
>> >> -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
>> >> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>> >>  asmlinkage unsigned long get_overflow_stack(void)
>> >>  {
>> >>         return (unsigned long)this_cpu_ptr(overflow_stack) +
>> >> --
>> >> 2.38.1
>> >>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> linux-riscv@lists.infradead.org
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Jisheng Zhang]

On Tue, Nov 29, 2022 at 06:35:14PM -0800, Palmer Dabbelt wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
> 
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>

Reviewed-by: Jisheng Zhang <jszhang@kernel.org>

> ---
>  arch/riscv/kernel/traps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/traps.c b/arch/riscv/kernel/traps.c
> index be54ccea8c47..acdfcacd7e57 100644
> --- a/arch/riscv/kernel/traps.c
> +++ b/arch/riscv/kernel/traps.c
> @@ -206,7 +206,7 @@ static DEFINE_PER_CPU(unsigned long [OVERFLOW_STACK_SIZE/sizeof(long)],
>   * shadow stack, handled_ kernel_ stack_ overflow(in kernel/entry.S) is used
>   * to get per-cpu overflow stack(get_overflow_stack).
>   */
> -long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)];
> +long shadow_stack[SHADOW_OVERFLOW_STACK_SIZE/sizeof(long)] __aligned(16);
>  asmlinkage unsigned long get_overflow_stack(void)
>  {
>  	return (unsigned long)this_cpu_ptr(overflow_stack) +
> -- 
> 2.38.1
> 

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[Palmer Dabbelt]

On Tue, 29 Nov 2022 18:35:14 -0800, Palmer Dabbelt wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
> 
> 

Applied, thanks!

[1/2] RISC-V: Align the shadow stack
      https://git.kernel.org/palmer/c/c3ec1e8964fb
[2/2] RISC-V: Add some comments about the shadow and overflow stacks
      https://git.kernel.org/palmer/c/de57ecc47610

Best regards,
-- 
Palmer Dabbelt <palmer@rivosinc.com>

Re: [PATCH 1/2] RISC-V: Align the shadow stack

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 29 Nov 2022 18:35:14 -0800 you wrote:
> The standard RISC-V ABIs all require 16-byte stack alignment.  We're
> only calling that one function on the shadow stack so I doubt it'd
> result in a real issue, but might as well keep this lined up.
> 
> Fixes: 31da94c25aea ("riscv: add VMAP_STACK overflow detection")
> Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> [...]

Here is the summary with links:
  - [1/2] RISC-V: Align the shadow stack
    https://git.kernel.org/riscv/c/b003b3b77d65
  - [2/2] RISC-V: Add some comments about the shadow and overflow stacks
    https://git.kernel.org/riscv/c/de57ecc47610

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html