[PATCH] riscv: Fix SMP when shadow call stacks are enabled

[PATCH] riscv: Fix SMP when shadow call stacks are enabled

[Samuel Holland]

This fixes two bugs in SCS initialization for secondary CPUs. First,
the SCS was not initialized at all in the spinwait boot path. Second,
the code for the SBI HSM path attempted to initialize the SCS before
enabling the MMU. However, that involves dereferencing the thread
pointer, which requires the MMU to be enabled.

Fix both issues by setting up the SCS in the common secondary entry
path, after enabling the MMU.

Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

 arch/riscv/kernel/head.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index b77397432403..76ace1e0b46f 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -154,7 +154,6 @@ secondary_start_sbi:
 	XIP_FIXUP_OFFSET a3
 	add a3, a3, a1
 	REG_L sp, (a3)
-	scs_load_current
 
 .Lsecondary_start_common:
 
@@ -165,6 +164,7 @@ secondary_start_sbi:
 	call relocate_enable_mmu
 #endif
 	call .Lsetup_trap_vector
+	scs_load_current
 	tail smp_callin
 #endif /* CONFIG_SMP */
 
-- 
2.42.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1485 bytes --]

On Tue, Nov 21, 2023 at 01:19:29PM -0800, Samuel Holland wrote:
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
> 
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.

I'm curious, mostly because I do not know much about the implemtnation
of the shadow call stack, but does it actually work correctly when the
kernel is built without mmu support?

> 
> Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
>  arch/riscv/kernel/head.S | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index b77397432403..76ace1e0b46f 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -154,7 +154,6 @@ secondary_start_sbi:
>  	XIP_FIXUP_OFFSET a3
>  	add a3, a3, a1
>  	REG_L sp, (a3)
> -	scs_load_current
>  
>  .Lsecondary_start_common:
>  
> @@ -165,6 +164,7 @@ secondary_start_sbi:
>  	call relocate_enable_mmu
>  #endif
>  	call .Lsetup_trap_vector
> +	scs_load_current
>  	tail smp_callin
>  #endif /* CONFIG_SMP */
>  
> -- 
> 2.42.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled

[Samuel Holland]

Hi Conor,

On 2023-11-23 8:06 AM, Conor Dooley wrote:
> On Tue, Nov 21, 2023 at 01:19:29PM -0800, Samuel Holland wrote:
>> This fixes two bugs in SCS initialization for secondary CPUs. First,
>> the SCS was not initialized at all in the spinwait boot path. Second,
>> the code for the SBI HSM path attempted to initialize the SCS before
>> enabling the MMU. However, that involves dereferencing the thread
>> pointer, which requires the MMU to be enabled.
>>
>> Fix both issues by setting up the SCS in the common secondary entry
>> path, after enabling the MMU.
> 
> I'm curious, mostly because I do not know much about the implemtnation
> of the shadow call stack, but does it actually work correctly when the
> kernel is built without mmu support?

I imagine it would work. The SCS implementation is purely software; it stores
the return address in a stack at `gp` instead of with the rest of local
variables at `sp`.

The problem here is that we are passing a pointer between CPUs with different
views of the virtual address space (i.e. the boot CPU sees the kernel at
0xffffffff80000000 while the CPU being brought up sees it at its physical
address), and then dereferencing it. If there is no MMU support, then the
virtual address space is identity mapped on all CPUs, and there is no problem.

Regards,
Samuel

>> Fixes: d1584d791a29 ("riscv: Implement Shadow Call Stack")
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>>  arch/riscv/kernel/head.S | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> index b77397432403..76ace1e0b46f 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -154,7 +154,6 @@ secondary_start_sbi:
>>  	XIP_FIXUP_OFFSET a3
>>  	add a3, a3, a1
>>  	REG_L sp, (a3)
>> -	scs_load_current
>>  
>>  .Lsecondary_start_common:
>>  
>> @@ -165,6 +164,7 @@ secondary_start_sbi:
>>  	call relocate_enable_mmu
>>  #endif
>>  	call .Lsetup_trap_vector
>> +	scs_load_current
>>  	tail smp_callin
>>  #endif /* CONFIG_SMP */
>>  
>> -- 
>> 2.42.0
>>


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled

[Sami Tolvanen]

Hi Samuel,

On Tue, Nov 21, 2023 at 1:20 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
>
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.

Thanks for the patch! Looks like my qemu setup doesn't hit this issue,
but nevertheless, the fix looks good to me.

Reviewed-by: Sami Tolvanen <samitolvanen@google.com>

Sami

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled

[patchwork-bot+linux-riscv]

Hello:

This patch was applied to riscv/linux.git (fixes)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Tue, 21 Nov 2023 13:19:29 -0800 you wrote:
> This fixes two bugs in SCS initialization for secondary CPUs. First,
> the SCS was not initialized at all in the spinwait boot path. Second,
> the code for the SBI HSM path attempted to initialize the SCS before
> enabling the MMU. However, that involves dereferencing the thread
> pointer, which requires the MMU to be enabled.
> 
> Fix both issues by setting up the SCS in the common secondary entry
> path, after enabling the MMU.
> 
> [...]

Here is the summary with links:
  - riscv: Fix SMP when shadow call stacks are enabled
    https://git.kernel.org/riscv/c/f40cab8e18ed

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



_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] riscv: Fix SMP when shadow call stacks are enabled

[Guo Ren]

On Fri, Dec 01, 2023 at 09:40:55AM -0800, Sami Tolvanen wrote:
> Hi Samuel,
> 
> On Tue, Nov 21, 2023 at 1:20 PM Samuel Holland
> <samuel.holland@sifive.com> wrote:
> >
> > This fixes two bugs in SCS initialization for secondary CPUs. First,
> > the SCS was not initialized at all in the spinwait boot path. Second,
> > the code for the SBI HSM path attempted to initialize the SCS before
> > enabling the MMU. However, that involves dereferencing the thread
> > pointer, which requires the MMU to be enabled.
> >
> > Fix both issues by setting up the SCS in the common secondary entry
> > path, after enabling the MMU.
> 
> Thanks for the patch! Looks like my qemu setup doesn't hit this issue,
> but nevertheless, the fix looks good to me.
Because there is no function call in relocate_enable_mmu :)

> 
> Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
> 
> Sami
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv