diff mbox series

[01/35] prctl: arch-agnostic prctl for shadow stack

Message ID 20230716-arm64-gcs-v1-1-bf567f93bba6@kernel.org (mailing list archive)
State New, archived
Headers show
Series arm64/gcs: Provide support for GCS at EL0 | expand

Commit Message

Mark Brown July 16, 2023, 9:50 p.m. UTC
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.

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>
---
 include/linux/mm.h         |  3 +++
 include/uapi/linux/prctl.h | 17 +++++++++++++++++
 kernel/sys.c               | 20 ++++++++++++++++++++
 3 files changed, 40 insertions(+)

Comments

Edgecombe, Rick P July 18, 2023, 5:45 p.m. UTC | #1
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?
Mark Brown July 18, 2023, 6:54 p.m. UTC | #2
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 mbox series

Patch

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;