2025-03-14T14:39:31-07:00, Deepak Gupta debug@rivosinc.com:
diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h @@ -14,7 +15,8 @@ struct kernel_clone_args; struct cfi_status { unsigned long ubcfi_en : 1; /* Enable for backward cfi. */
- unsigned long rsvd : ((sizeof(unsigned long) * 8) - 1);
- unsigned long ubcfi_locked : 1;
- unsigned long rsvd : ((sizeof(unsigned long) * 8) - 2);
The rsvd field shouldn't be necessary as the container for the bitfield is 'unsigned long' sized.
Why don't we use bools here, though? It might produce a better binary and we're not hurting for struct size.
diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c @@ -24,6 +24,16 @@ bool is_shstk_enabled(struct task_struct *task) +bool is_shstk_allocated(struct task_struct *task) +{
- return task->thread_info.user_cfi_state.shdw_stk_base ? true : false;
I think that the following is clearer:
return task->thread_info.user_cfi_state.shdw_stk_base
(Similar for all other implicit conversion ternaries.)
@@ -42,6 +52,26 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr) +void set_shstk_status(struct task_struct *task, bool enable) +{
- if (!cpu_supports_shadow_stack())
return;- task->thread_info.user_cfi_state.ubcfi_en = enable ? 1 : 0;
- if (enable)
task->thread.envcfg |= ENVCFG_SSE;- else
task->thread.envcfg &= ~ENVCFG_SSE;- csr_write(CSR_ENVCFG, task->thread.envcfg);
There is a new helper we could reuse for this:
envcfg_update_bits(task, ENVCFG_SSE, enable ? ENVCFG_SSE : 0);
+} @@ -262,3 +292,83 @@ void shstk_release(struct task_struct *tsk) +int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status) +{
- /* Request is to enable shadow stack and shadow stack is not enabled already */
- if (enable_shstk && !is_shstk_enabled(t)) {
/* shadow stack was allocated and enable request again* no need to support such usecase and return EINVAL.*/if (is_shstk_allocated(t))return -EINVAL;size = calc_shstk_size(0);addr = allocate_shadow_stack(0, size, 0, false);
Why don't we use the userspace-allocated stack?
I'm completely missing the design idea here... Userspace has absolute over the shadow stack pointer CSR, so we don't need to do much in Linux:
1. interface to set up page tables with -W- PTE and 2. interface to control senvcfg.SSE.
Userspace can do the rest.
+int arch_lock_shadow_stack_status(struct task_struct *task,
unsigned long arg)+{
- /* If shtstk not supported or not enabled on task, nothing to lock here */
- if (!cpu_supports_shadow_stack() ||
!is_shstk_enabled(task) || arg != 0)return -EINVAL;
The task might want to prevent shadow stack from being enabled?
Thanks.