2025-03-14T14:39:31-07:00, Deepak Gupta <de...@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.
