| Message ID | 20250314-v5_user_cfi_series-v12-12-e51202b53138@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | riscv control-flow integrity for usermode | expand |
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.
2025-04-23T21:44:09-07:00, Deepak Gupta <debug@rivosinc.com>: > On Thu, Apr 10, 2025 at 11:45:58AM +0200, Radim Krčmář wrote: >>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. > > If you remember one of the previous patch discussion, this goes into > `thread_info` Don't want to bloat it. Even if we end shoving into task_struct, > don't want to bloat that either. I can just convert it into bitmask if > bitfields are an eyesore here. "unsigned long rsvd : ((sizeof(unsigned long) * 8) - 2);" is an eyesore that defines exactly the same as the two lines alone unsigned long ubcfi_en : 1; unsigned long ubcfi_locked : 1; That one should be removed. If we have only 4 bits in 4/8 bytes, then bitfields do generate worse code than 4 bools and a 0/4 byte hole. The struct size stays the same. I don't care much about the switch to bools, though, because this code is not called often. >>> @@ -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. > > Design is like following: > > When a user task wants to enable shadow stack for itself, it has to issue > a syscall to kernel (like this prctl). Now it can be done independently by > user task by first issuing `map_shadow_stack`, then asking kernel to light > up envcfg bit and eventually when return to usermode happens, it can write > to CSR. It is no different from doing all of the above together in single > `prctl` call. They are equivalent in that nature. > > Background is that x86 followed this because x86 had workloads/binaries/ > functions with (deep)recursive functions and thus by default were forced > to always allocate shadow stack to be of the same size as data stack. To > reduce burden on userspace for determining and then allocating same size > (size of data stack) shadow stack, prctl would do the job of calculating > default shadow stack size (and reduce programming error in usermode). arm64 > followed the suite. I don't want to find out what's the compatiblity issues > we will see and thus just following the suite (given that both approaches > are equivalent). Take a look at static `calc_shstk_size(unsigned long size)`. > > Coming back to your question of why not allowing userspace to manage its > own shadow stack. Answer is that it can manage its own shadow stack. If it > does, it just have to be aware of size its allocating for shadow stack. It's just that userspace cannot prevent allocation of the default stack when enabling it, which is the weird part to me. The allocate and enable syscalls could have been nicely composable. > There is already a patch series going on to manage this using clone3. > https://lore.kernel.org/all/20250408-clone3-shadow-stack-v15-4-3fa245c6e3be@kernel.org/ A new ioctl does seem to solve most of the practical issues, thanks. > I fully expect green thread implementations in rust/go or swapcontext > based thread management doing this on their own. > > Current design is to ensure existing apps dont have to change a lot in > userspace and by default kernel gives compatibility. Anyone else wanting > to optimize the usage of shadow stack can do so with current design. Right, changing rlimit_stack around shadow stack allocation is not the most elegant way, but it does work. >>> +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? > > But Why would it want to do that? Task can simply not issue the prctl. There > are glibc tunables as well using which it can be disabled. The task might do it as some last resort to prevent a buggy code from enabling shadow stacks that would just crash. Or whatever complicated reason userspace can think of. It's more the other way around. I wonder why we're removing this option when we don't really care what userspace does to itself. I think it's complicating the kernel without an obvious gain.
On Thu, Apr 24, 2025 at 03:36:54PM +0200, Radim Krčmář wrote: >2025-04-23T21:44:09-07:00, Deepak Gupta <debug@rivosinc.com>: >> On Thu, Apr 10, 2025 at 11:45:58AM +0200, Radim Krčmář wrote: >>>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. >> >> If you remember one of the previous patch discussion, this goes into >> `thread_info` Don't want to bloat it. Even if we end shoving into task_struct, >> don't want to bloat that either. I can just convert it into bitmask if >> bitfields are an eyesore here. > > "unsigned long rsvd : ((sizeof(unsigned long) * 8) - 2);" > >is an eyesore that defines exactly the same as the two lines alone > > unsigned long ubcfi_en : 1; > unsigned long ubcfi_locked : 1; > >That one should be removed. > >If we have only 4 bits in 4/8 bytes, then bitfields do generate worse >code than 4 bools and a 0/4 byte hole. The struct size stays the same. > >I don't care much about the switch to bools, though, because this code >is not called often. I'll remove the bitfields, have single `unsigned long cfi_control_state` And do `#define RISCV_UBCFI_EN 1` and so on. > >>>> @@ -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. >> >> Design is like following: >> >> When a user task wants to enable shadow stack for itself, it has to issue >> a syscall to kernel (like this prctl). Now it can be done independently by >> user task by first issuing `map_shadow_stack`, then asking kernel to light >> up envcfg bit and eventually when return to usermode happens, it can write >> to CSR. It is no different from doing all of the above together in single >> `prctl` call. They are equivalent in that nature. >> >> Background is that x86 followed this because x86 had workloads/binaries/ >> functions with (deep)recursive functions and thus by default were forced >> to always allocate shadow stack to be of the same size as data stack. To >> reduce burden on userspace for determining and then allocating same size >> (size of data stack) shadow stack, prctl would do the job of calculating >> default shadow stack size (and reduce programming error in usermode). arm64 >> followed the suite. I don't want to find out what's the compatiblity issues >> we will see and thus just following the suite (given that both approaches >> are equivalent). Take a look at static `calc_shstk_size(unsigned long size)`. >> >> Coming back to your question of why not allowing userspace to manage its >> own shadow stack. Answer is that it can manage its own shadow stack. If it >> does, it just have to be aware of size its allocating for shadow stack. > >It's just that userspace cannot prevent allocation of the default stack >when enabling it, which is the weird part to me. >The allocate and enable syscalls could have been nicely composable. > >> There is already a patch series going on to manage this using clone3. >> https://lore.kernel.org/all/20250408-clone3-shadow-stack-v15-4-3fa245c6e3be@kernel.org/ > >A new ioctl does seem to solve most of the practical issues, thanks. > >> I fully expect green thread implementations in rust/go or swapcontext >> based thread management doing this on their own. >> >> Current design is to ensure existing apps dont have to change a lot in >> userspace and by default kernel gives compatibility. Anyone else wanting >> to optimize the usage of shadow stack can do so with current design. > >Right, changing rlimit_stack around shadow stack allocation is not the >most elegant way, but it does work. > >>>> +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? >> >> But Why would it want to do that? Task can simply not issue the prctl. There >> are glibc tunables as well using which it can be disabled. > >The task might do it as some last resort to prevent a buggy code from >enabling shadow stacks that would just crash. Or whatever complicated >reason userspace can think of. > >It's more the other way around. I wonder why we're removing this option >when we don't really care what userspace does to itself. >I think it's complicating the kernel without an obvious gain. It just feels wierd. There isn't anything like this for other features lit-up via envcfg. Does hwprobe allow this on per-task basis? I'll look into it.
diff --git a/arch/riscv/include/asm/usercfi.h b/arch/riscv/include/asm/usercfi.h index 82d28ac98d76..c4dcd256f19a 100644 --- a/arch/riscv/include/asm/usercfi.h +++ b/arch/riscv/include/asm/usercfi.h @@ -7,6 +7,7 @@ #ifndef __ASSEMBLY__ #include <linux/types.h> +#include <linux/prctl.h> struct task_struct; struct kernel_clone_args; @@ -14,7 +15,8 @@ struct kernel_clone_args; #ifdef CONFIG_RISCV_USER_CFI 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); unsigned long user_shdw_stk; /* Current user shadow stack pointer */ unsigned long shdw_stk_base; /* Base address of shadow stack */ unsigned long shdw_stk_size; /* size of shadow stack */ @@ -27,6 +29,12 @@ void set_shstk_base(struct task_struct *task, unsigned long shstk_addr, unsigned unsigned long get_shstk_base(struct task_struct *task, unsigned long *size); void set_active_shstk(struct task_struct *task, unsigned long shstk_addr); bool is_shstk_enabled(struct task_struct *task); +bool is_shstk_locked(struct task_struct *task); +bool is_shstk_allocated(struct task_struct *task); +void set_shstk_lock(struct task_struct *task); +void set_shstk_status(struct task_struct *task, bool enable); + +#define PR_SHADOW_STACK_SUPPORTED_STATUS_MASK (PR_SHADOW_STACK_ENABLE) #else @@ -42,6 +50,14 @@ bool is_shstk_enabled(struct task_struct *task); #define is_shstk_enabled(task) false +#define is_shstk_locked(task) false + +#define is_shstk_allocated(task) false + +#define set_shstk_lock(task) + +#define set_shstk_status(task, enable) + #endif /* CONFIG_RISCV_USER_CFI */ #endif /* __ASSEMBLY__ */ diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c index 99acb6342a37..cd11667593fe 100644 --- a/arch/riscv/kernel/process.c +++ b/arch/riscv/kernel/process.c @@ -153,6 +153,14 @@ void start_thread(struct pt_regs *regs, unsigned long pc, regs->epc = pc; regs->sp = sp; + /* + * clear shadow stack state on exec. + * libc will set it later via prctl. + */ + set_shstk_status(current, false); + set_shstk_base(current, 0, 0); + set_active_shstk(current, 0); + #ifdef CONFIG_64BIT regs->status &= ~SR_UXL; diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c index 73cf87dab186..b93b324eed26 100644 --- a/arch/riscv/kernel/usercfi.c +++ b/arch/riscv/kernel/usercfi.c @@ -24,6 +24,16 @@ bool is_shstk_enabled(struct task_struct *task) return task->thread_info.user_cfi_state.ubcfi_en ? true : false; } +bool is_shstk_allocated(struct task_struct *task) +{ + return task->thread_info.user_cfi_state.shdw_stk_base ? true : false; +} + +bool is_shstk_locked(struct task_struct *task) +{ + return task->thread_info.user_cfi_state.ubcfi_locked ? true : false; +} + void set_shstk_base(struct task_struct *task, unsigned long shstk_addr, unsigned long size) { task->thread_info.user_cfi_state.shdw_stk_base = shstk_addr; @@ -42,6 +52,26 @@ void set_active_shstk(struct task_struct *task, unsigned long shstk_addr) task->thread_info.user_cfi_state.user_shdw_stk = 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); +} + +void set_shstk_lock(struct task_struct *task) +{ + task->thread_info.user_cfi_state.ubcfi_locked = 1; +} + /* * If size is 0, then to be compatible with regular stack we want it to be as big as * regular stack. Else PAGE_ALIGN it and return back @@ -262,3 +292,83 @@ void shstk_release(struct task_struct *tsk) vm_munmap(base, size); set_shstk_base(tsk, 0, 0); } + +int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *status) +{ + unsigned long bcfi_status = 0; + + if (!cpu_supports_shadow_stack()) + return -EINVAL; + + /* this means shadow stack is enabled on the task */ + bcfi_status |= (is_shstk_enabled(t) ? PR_SHADOW_STACK_ENABLE : 0); + + return copy_to_user(status, &bcfi_status, sizeof(bcfi_status)) ? -EFAULT : 0; +} + +int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status) +{ + unsigned long size = 0, addr = 0; + bool enable_shstk = false; + + if (!cpu_supports_shadow_stack()) + return -EINVAL; + + /* Reject unknown flags */ + if (status & ~PR_SHADOW_STACK_SUPPORTED_STATUS_MASK) + return -EINVAL; + + /* bcfi status is locked and further can't be modified by user */ + if (is_shstk_locked(t)) + return -EINVAL; + + enable_shstk = status & PR_SHADOW_STACK_ENABLE; + /* 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); + if (IS_ERR_VALUE(addr)) + return -ENOMEM; + set_shstk_base(t, addr, size); + set_active_shstk(t, addr + size); + } + + /* + * If a request to disable shadow stack happens, let's go ahead and release it + * Although, if CLONE_VFORKed child did this, then in that case we will end up + * not releasing the shadow stack (because it might be needed in parent). Although + * we will disable it for VFORKed child. And if VFORKed child tries to enable again + * then in that case, it'll get entirely new shadow stack because following condition + * are true + * - shadow stack was not enabled for vforked child + * - shadow stack base was anyways pointing to 0 + * This shouldn't be a big issue because we want parent to have availability of shadow + * stack whenever VFORKed child releases resources via exit or exec but at the same + * time we want VFORKed child to break away and establish new shadow stack if it desires + * + */ + if (!enable_shstk) + shstk_release(t); + + set_shstk_status(t, enable_shstk); + return 0; +} + +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; + + set_shstk_lock(task); + + return 0; +}
Implement architecture agnostic prctls() interface for setting and getting shadow stack status. prctls implemented are PR_GET_SHADOW_STACK_STATUS, PR_SET_SHADOW_STACK_STATUS and PR_LOCK_SHADOW_STACK_STATUS. As part of PR_SET_SHADOW_STACK_STATUS/PR_GET_SHADOW_STACK_STATUS, only PR_SHADOW_STACK_ENABLE is implemented because RISCV allows each mode to write to their own shadow stack using `sspush` or `ssamoswap`. PR_LOCK_SHADOW_STACK_STATUS locks current configuration of shadow stack enabling. Signed-off-by: Deepak Gupta <debug@rivosinc.com> --- arch/riscv/include/asm/usercfi.h | 18 ++++++- arch/riscv/kernel/process.c | 8 +++ arch/riscv/kernel/usercfi.c | 110 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 1 deletion(-)