| Message ID | 20230716-arm64-gcs-v1-1-bf567f93bba6@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | arm64/gcs: Provide support for GCS at EL0 | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/tree_selection | fail | Failed to apply to next/pending-fixes, riscv/for-next or riscv/master |
On Sun, 2023-07-16 at 22:50 +0100, Mark Brown wrote: > From: Deepak Gupta <debug@rivosinc.com> > > Three architectures (x86, aarch64, riscv) have announced support for > shadow stack. This patch adds arch-agnostic prtcl support to enable > /disable/get/set status of shadow stack and forward control (landing > pad) > flow cfi statuses. What is this about forward control flow? Seems to be just about shadow stack. > > New prctls are > - PR_GET_SHADOW_STACK_STATUS, PR_SET_SHADOW_STACK_STATUS > > Signed-off-by: Deepak Gupta <debug@rivosinc.com> > [Rebased onto current kernels, renumbering to track other allocations > already upstream, dropping indirect LP, updating to pass arg to set > by value, fix missing prototypes for weak functions and update > title. > -- broonie] > Signed-off-by: Mark Brown <broonie@kernel.org> This is similar to the arch_prctl() thing x86 does, but I actually see a fair amount of differences: 1. PR_SET_SHADOW_STACK_STATUS seems like a strange name for the thing actually doing the whole enabling of the feature which involves allocating memory, etc. And in the future a growing array of different things (enabling push, write, etc). 2. x86 only allows one enabling/disabling operation at a time. So you can't enable shadow stack AND WRSS with one syscall, for example. This is to make it so it's clear which operation failed. Also, since some features depend on others (WRSS), there would need to be some ordering and rollback logic. There was some discussion about a batch enabling arch_prctl() that could report failures independently, but it was deemed premature optimization. 3. It only allows you to lock the whole feature, and not individual subfeatures. For things like WRSS, it came up that there might be an elf bit, like the shadow stack one, but that works a bit different. Instead of only enabling shadow stack when ALL DSOs support the feature, it would want to be enabled if ANY DSOs require it. So userspace might want to do something like lock shadow stack, but leave WRSS unlocked in case a dlopen() call came across a WRSS-requiring DSO. 4. To support CRIU, there needed to be a ptrace-only unlock feature. The arch_prctl() has a special ptrace route to enforce that this unlock is only coming from ptrace. Is there some way to do this with a regular prctl()? 5. I see in the next patch there is hinted support for write and push as well (although I can't find the implementation in the patches, am I missing it?). X86 has something close enough to write, but not push. What is the idea for when the features don't exactly match? I think when Deepak originally brought up this unified prctl-based interface, it seemed far away before we could tell if it *could* be unified. Do either of you have any thoughts on whether the above points could be incorporated?
On Tue, Jul 18, 2023 at 05:45:01PM +0000, Edgecombe, Rick P wrote: > On Sun, 2023-07-16 at 22:50 +0100, Mark Brown wrote: > > Three architectures (x86, aarch64, riscv) have announced support for > > shadow stack. This patch adds arch-agnostic prtcl support to enable > > /disable/get/set status of shadow stack and forward control (landing > > pad) > > flow cfi statuses. > What is this about forward control flow? Seems to be just about shadow > stack. Sorry, that's the original commit message - the original version of this also had support for controlling landing pads but I don't need that and cut them out of the series. I forgot to update that bit of the commit message. > > [Rebased onto current kernels, renumbering to track other allocations > > already upstream, dropping indirect LP, updating to pass arg to set > > by value, fix missing prototypes for weak functions and update > > title. > > -- broonie] > 1. PR_SET_SHADOW_STACK_STATUS seems like a strange name for the thing > actually doing the whole enabling of the feature which involves > allocating memory, etc. And in the future a growing array of different > things (enabling push, write, etc). I have no strong opinion on naming here. _MODE? I didn't find any discussions around this in the > 2. x86 only allows one enabling/disabling operation at a time. So you > can't enable shadow stack AND WRSS with one syscall, for example. This > is to make it so it's clear which operation failed. Also, since some > features depend on others (WRSS), there would need to be some ordering > and rollback logic. There was some discussion about a batch enabling > arch_prctl() that could report failures independently, but it was > deemed premature optimization. I did see that the x86 implementation required a call per flag, the logic wasn't hugely obvious there - it didn't seem super helpful. There's nothing stopping userspace turning one flag at a time if it wants to, we just don't require it. I wasn't overly concerned about the rollback logic since I was anticipating that the main complexity is the base enable and allocate, everything else would just be storing a mode. We can implement things with the one bit per call approach, I just didn't see much upside to it. Perhaps I'm missing some case though. > 3. It only allows you to lock the whole feature, and not individual > subfeatures. For things like WRSS, it came up that there might be an > elf bit, like the shadow stack one, but that works a bit different. > Instead of only enabling shadow stack when ALL DSOs support the > feature, it would want to be enabled if ANY DSOs require it. So > userspace might want to do something like lock shadow stack, but leave > WRSS unlocked in case a dlopen() call came across a WRSS-requiring DSO. We could add either a second argument with the lock or a separate lock prctl() and matching query which takes the same bitmask, being able to lock per feature does give more flexibility to userspace in how we do the locking and isn't hugely more costly to implement. My model for locking had been that there would be a final decision on what the features should be, I was modelling "can enable" as equivalent access to "is enabled" when it came to what was locked. > 4. To support CRIU, there needed to be a ptrace-only unlock feature. > The arch_prctl() has a special ptrace route to enforce that this unlock > is only coming from ptrace. Is there some way to do this with a regular > prctl()? For arm64 we need to add a regset to expose the GCS pointer anyway so the GCS mode is in there, though at the minute we prevent any changes at all via that mechanism it could be implemented later. I'm not aware of any way for prctl() to tell if it is being invoked via ptrace so that'd need to be dealt with somehow. > 5. I see in the next patch there is hinted support for write and push > as well (although I can't find the implementation in the patches, am I > missing it?). X86 has something close enough to write, but not push. > What is the idea for when the features don't exactly match? The implementation is in "arm64/gcs: Implement shadow stack prctl() interface", it just boils down to turning on or off a register bit. > I think when Deepak originally brought up this unified prctl-based > interface, it seemed far away before we could tell if it *could* be > unified. Do either of you have any thoughts on whether the above points > could be incorporated? Other than the issue with CRIU I don't see any huge difficulty.
diff --git a/include/linux/mm.h b/include/linux/mm.h index 97eddc83d19c..bf16edf2fcd9 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -3947,4 +3947,7 @@ static inline void accept_memory(phys_addr_t start, phys_addr_t end) #endif +int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *status); +int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status); + #endif /* _LINUX_MM_H */ diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h index 3c36aeade991..9fdc77fa2bfe 100644 --- a/include/uapi/linux/prctl.h +++ b/include/uapi/linux/prctl.h @@ -305,4 +305,21 @@ struct prctl_mm_map { # define PR_RISCV_V_VSTATE_CTRL_NEXT_MASK 0xc # define PR_RISCV_V_VSTATE_CTRL_MASK 0x1f +/* + * get shadow stack status for current thread. Assumes shadow stack is min 4 byte aligned. + * Note shadow stack can be 8 byte aligned on 64bit. + * Lower 2 bits can give status of locked and enabled/disabled. + * size and address range can be obtained via /proc/maps. get_shadow_stack_status will + * return base of shadow stack. + */ +#define PR_GET_SHADOW_STACK_STATUS 71 +/* + * set shadow stack status for current thread (including enabling, disabling or locking) + * note that it will only set the status and setup of the shadow stack. Allocating shadow + * stack should be done separately using mmap. + */ +#define PR_SET_SHADOW_STACK_STATUS 72 +# define PR_SHADOW_STACK_LOCK (1UL << 0) +# define PR_SHADOW_STACK_ENABLE (1UL << 1) + #endif /* _LINUX_PRCTL_H */ diff --git a/kernel/sys.c b/kernel/sys.c index 05f838929e72..ebf9ea5f0fae 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2302,6 +2302,16 @@ int __weak arch_prctl_spec_ctrl_set(struct task_struct *t, unsigned long which, return -EINVAL; } +int __weak arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *status) +{ + return -EINVAL; +} + +int __weak arch_set_shadow_stack_status(struct task_struct *t, unsigned long status) +{ + return -EINVAL; +} + #define PR_IO_FLUSHER (PF_MEMALLOC_NOIO | PF_LOCAL_THROTTLE) #ifdef CONFIG_ANON_VMA_NAME @@ -2720,6 +2730,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, case PR_RISCV_V_GET_CONTROL: error = RISCV_V_GET_CONTROL(); break; + case PR_GET_SHADOW_STACK_STATUS: + if (arg3 || arg4 || arg5) + return -EINVAL; + error = arch_get_shadow_stack_status(me, (unsigned long __user *) arg2); + break; + case PR_SET_SHADOW_STACK_STATUS: + if (arg3 || arg4 || arg5) + return -EINVAL; + error = arch_set_shadow_stack_status(me, arg2); + break; default: error = -EINVAL; break;