diff mbox series

riscv: Fix SMP when shadow call stacks are enabled

Message ID 20231121211958.3158576-1-samuel.holland@sifive.com
State New
Headers
Series riscv: Fix SMP when shadow call stacks are enabled |

Commit Message

Samuel Holland Nov. 21, 2023, 9:19 p.m. UTC 
  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(-)

Comments

Conor Dooley Nov. 23, 2023, 2:06 p.m. UTC | #1 
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
>
Samuel Holland Nov. 26, 2023, 12:06 a.m. UTC | #2 
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
>>
Sami Tolvanen Dec. 1, 2023, 5:40 p.m. UTC | #3 
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
patchwork-bot+linux-riscv@kernel.org Dec. 7, 2023, 3:20 p.m. UTC | #4 
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!
Guo Ren Dec. 8, 2023, 1:50 a.m. UTC | #5 
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
diff mbox series

Patch

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 */