diff mbox series

[v3,24/37] x86: Introduce userspace API for CET enabling

Message ID 20221104223604.29615-25-rick.p.edgecombe@intel.com (mailing list archive)
State New
Headers show
Series Shadow stacks for userspace | expand

Commit Message

Edgecombe, Rick P Nov. 4, 2022, 10:35 p.m. UTC 
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

Add three new arch_prctl() handles:

 - ARCH_CET_ENABLE/DISABLE enables or disables the specified
   feature. Returns 0 on success or an error.

 - ARCH_CET_LOCK prevents future disabling or enabling of the
   specified feature. Returns 0 on success or an error

The features are handled per-thread and inherited over fork(2)/clone(2),
but reset on exec().

This is preparation patch. It does not implement any features.

Tested-by: Pengfei Xu <pengfei.xu@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
[tweaked with feedback from tglx]
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

---

v3:
 - Move shstk.c Makefile changes earlier (Kees)
 - Add #ifdef around features_locked and features (Kees)
 - Encapsulate features reset earlier in reset_thread_features() so
   features and features_locked are not referenced in code that would be
   compiled !CONFIG_X86_USER_SHADOW_STACK. (Kees)
 - Fix typo in commit log (Kees)
 - Switch arch_prctl() numbers to avoid conflict with LAM

v2:
 - Only allow one enable/disable per call (tglx)
 - Return error code like a normal arch_prctl() (Alexander Potapenko)
 - Make CET only (tglx)

 arch/x86/include/asm/cet.h        | 21 +++++++++++++++
 arch/x86/include/asm/processor.h  |  5 ++++
 arch/x86/include/uapi/asm/prctl.h |  6 +++++
 arch/x86/kernel/Makefile          |  2 ++
 arch/x86/kernel/process_64.c      |  7 ++++-
 arch/x86/kernel/shstk.c           | 44 +++++++++++++++++++++++++++++++
 6 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/include/asm/cet.h
 create mode 100644 arch/x86/kernel/shstk.c

Comments

Peter Zijlstra Nov. 15, 2022, 12:26 p.m. UTC | #1 
On Fri, Nov 04, 2022 at 03:35:51PM -0700, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> 
> Add three new arch_prctl() handles:
> 
>  - ARCH_CET_ENABLE/DISABLE enables or disables the specified
>    feature. Returns 0 on success or an error.
> 
>  - ARCH_CET_LOCK prevents future disabling or enabling of the
>    specified feature. Returns 0 on success or an error
> 
> The features are handled per-thread and inherited over fork(2)/clone(2),
> but reset on exec().
> 
> This is preparation patch. It does not implement any features.

Urgh... so much for sharing with other architectures I suppose :/

The ARM64 BTI thing is very similar to IBT (except I think their
approach to the legacy bitmap is much saner).

Given that IBT isn't supported and needs the whole legacy bitmap mess,
do we really want to call this CET ? Why not just make a Shadow Stack
API and tackle IBT independently.
Peter Zijlstra Nov. 15, 2022, 1:03 p.m. UTC | #2 
On Tue, Nov 15, 2022 at 01:26:23PM +0100, Peter Zijlstra wrote:
> On Fri, Nov 04, 2022 at 03:35:51PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > 
> > Add three new arch_prctl() handles:
> > 
> >  - ARCH_CET_ENABLE/DISABLE enables or disables the specified
> >    feature. Returns 0 on success or an error.
> > 
> >  - ARCH_CET_LOCK prevents future disabling or enabling of the
> >    specified feature. Returns 0 on success or an error
> > 
> > The features are handled per-thread and inherited over fork(2)/clone(2),
> > but reset on exec().
> > 
> > This is preparation patch. It does not implement any features.
> 
> Urgh... so much for sharing with other architectures I suppose :/
> 
> The ARM64 BTI thing is very similar to IBT (except I think their
> approach to the legacy bitmap is much saner).
> 
> Given that IBT isn't supported and needs the whole legacy bitmap mess,
> do we really want to call this CET ? Why not just make a Shadow Stack
> API and tackle IBT independently.

On that; ARM64 exposes PROT_BTI (to be used by mprotect()) and have an
ELF_ARM64_BTI note for the loader to bootstrap things.

We could co-opt that same interface and instead of flipping actual PTE
bits, have this thing manage the legacy bitmap -- basically have the
legacy bitmap function as an external PTE bit array (in inverse).

Basically, have every page mapped PROT_EXEC set the bit in the legacy
bitmap while every page mapped PROT_EXEC|PROT_BTI will have the legacy
bitmap bit to 0.

And as long as there is a single 0 in the bitmap, the feature is
enabled.

(obviously we can delay allocating the bitmap until the first PROT_EXEC
mapping that lacks PROT_BTI)
Peter Zijlstra Nov. 15, 2022, 2:25 p.m. UTC | #3 
On Fri, Nov 04, 2022 at 03:35:51PM -0700, Rick Edgecombe wrote:

>  arch/x86/include/asm/cet.h        | 21 +++++++++++++++
>  arch/x86/kernel/shstk.c           | 44 +++++++++++++++++++++++++++++++

You see what's going wrong there? Eradicate the CET...
Edgecombe, Rick P Nov. 15, 2022, 8:55 p.m. UTC | #4 
On Tue, 2022-11-15 at 14:03 +0100, Peter Zijlstra wrote:
> On Tue, Nov 15, 2022 at 01:26:23PM +0100, Peter Zijlstra wrote:
> > On Fri, Nov 04, 2022 at 03:35:51PM -0700, Rick Edgecombe wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > 
> > > Add three new arch_prctl() handles:
> > > 
> > >   - ARCH_CET_ENABLE/DISABLE enables or disables the specified
> > >     feature. Returns 0 on success or an error.
> > > 
> > >   - ARCH_CET_LOCK prevents future disabling or enabling of the
> > >     specified feature. Returns 0 on success or an error
> > > 
> > > The features are handled per-thread and inherited over
> > > fork(2)/clone(2),
> > > but reset on exec().
> > > 
> > > This is preparation patch. It does not implement any features.
> > 
> > Urgh... so much for sharing with other architectures I suppose :/
> > 
> > The ARM64 BTI thing is very similar to IBT (except I think their
> > approach to the legacy bitmap is much saner).
> > 
> > Given that IBT isn't supported and needs the whole legacy bitmap
> > mess,
> > do we really want to call this CET ? Why not just make a Shadow
> > Stack
> > API and tackle IBT independently.
> 
> On that; ARM64 exposes PROT_BTI (to be used by mprotect()) and have
> an
> ELF_ARM64_BTI note for the loader to bootstrap things.
> 
> We could co-opt that same interface and instead of flipping actual
> PTE
> bits, have this thing manage the legacy bitmap -- basically have the
> legacy bitmap function as an external PTE bit array (in inverse).
> 
> Basically, have every page mapped PROT_EXEC set the bit in the legacy
> bitmap while every page mapped PROT_EXEC|PROT_BTI will have the
> legacy
> bitmap bit to 0.
> 
> And as long as there is a single 0 in the bitmap, the feature is
> enabled.
> 
> (obviously we can delay allocating the bitmap until the first
> PROT_EXEC
> mapping that lacks PROT_BTI)

This is an interesting idea. I'll have to think a little more on it.

One non-impossible issue would be setting IBT in the MSR late. Each
thread would have to be interrupted and have it set, while no new
threads are created. Maybe this is easy and I just don't know how to do
it.

The other thing is there would be overhead compared to an IBT
implementation with a separate interface from BTI. Would have to look
at the tradeoffs.
Edgecombe, Rick P Nov. 15, 2022, 8:55 p.m. UTC | #5 
On Tue, 2022-11-15 at 15:25 +0100, Peter Zijlstra wrote:
> On Fri, Nov 04, 2022 at 03:35:51PM -0700, Rick Edgecombe wrote:
> 
> >   arch/x86/include/asm/cet.h        | 21 +++++++++++++++
> >   arch/x86/kernel/shstk.c           | 44
> > +++++++++++++++++++++++++++++++
> 
> You see what's going wrong there? Eradicate the CET...

Yep, will do. Thanks.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
new file mode 100644
index 000000000000..a2f3c6e06ef5
--- /dev/null
+++ b/arch/x86/include/asm/cet.h
@@ -0,0 +1,21 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_X86_CET_H
+#define _ASM_X86_CET_H
+
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+
+struct task_struct;
+
+#ifdef CONFIG_X86_USER_SHADOW_STACK
+long cet_prctl(struct task_struct *task, int option, unsigned long features);
+void reset_thread_features(void);
+#else
+static inline long cet_prctl(struct task_struct *task, int option,
+			     unsigned long features) { return -EINVAL; }
+static inline void reset_thread_features(void) {}
+#endif /* CONFIG_X86_USER_SHADOW_STACK */
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_X86_CET_H */
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 67c9d73b31fa..ca66d320a263 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -530,6 +530,11 @@  struct thread_struct {
 	 */
 	u32			pkru;
 
+#ifdef CONFIG_X86_USER_SHADOW_STACK
+	unsigned long		features;
+	unsigned long		features_locked;
+#endif
+
 	/* Floating point and extended processor state */
 	struct fpu		fpu;
 	/*
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
index 500b96e71f18..2dae9997ee17 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -20,4 +20,10 @@ 
 #define ARCH_MAP_VDSO_32		0x2002
 #define ARCH_MAP_VDSO_64		0x2003
 
+/* Don't use 0x3001-0x3004 because of old glibcs */
+
+#define ARCH_CET_ENABLE			0x5001
+#define ARCH_CET_DISABLE		0x5002
+#define ARCH_CET_LOCK			0x5003
+
 #endif /* _ASM_X86_PRCTL_H */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index f901658d9f7c..fbb1cb34188d 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -143,6 +143,8 @@  obj-$(CONFIG_AMD_MEM_ENCRYPT)		+= sev.o
 
 obj-$(CONFIG_CFI_CLANG)			+= cfi.o
 
+obj-$(CONFIG_X86_USER_SHADOW_STACK)	+= shstk.o
+
 ###
 # 64 bit specific files
 ifeq ($(CONFIG_X86_64),y)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 6b3418bff326..17fec059317c 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -514,6 +514,8 @@  start_thread_common(struct pt_regs *regs, unsigned long new_ip,
 		load_gs_index(__USER_DS);
 	}
 
+	reset_thread_features();
+
 	loadsegment(fs, 0);
 	loadsegment(es, _ds);
 	loadsegment(ds, _ds);
@@ -830,7 +832,10 @@  long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
 	case ARCH_MAP_VDSO_64:
 		return prctl_map_vdso(&vdso_image_64, arg2);
 #endif
-
+	case ARCH_CET_ENABLE:
+	case ARCH_CET_DISABLE:
+	case ARCH_CET_LOCK:
+		return cet_prctl(task, option, arg2);
 	default:
 		ret = -EINVAL;
 		break;
diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
new file mode 100644
index 000000000000..ed6f25cc07c5
--- /dev/null
+++ b/arch/x86/kernel/shstk.c
@@ -0,0 +1,44 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * shstk.c - Intel shadow stack support
+ *
+ * Copyright (c) 2021, Intel Corporation.
+ * Yu-cheng Yu <yu-cheng.yu@intel.com>
+ */
+
+#include <linux/sched.h>
+#include <linux/bitops.h>
+#include <asm/prctl.h>
+
+void reset_thread_features(void)
+{
+	current->thread.features = 0;
+	current->thread.features_locked = 0;
+}
+
+long cet_prctl(struct task_struct *task, int option, unsigned long features)
+{
+	if (option == ARCH_CET_LOCK) {
+		task->thread.features_locked |= features;
+		return 0;
+	}
+
+	/* Don't allow via ptrace */
+	if (task != current)
+		return -EINVAL;
+
+	/* Do not allow to change locked features */
+	if (features & task->thread.features_locked)
+		return -EPERM;
+
+	/* Only support enabling/disabling one feature at a time. */
+	if (hweight_long(features) > 1)
+		return -EINVAL;
+
+	if (option == ARCH_CET_DISABLE) {
+		return -EINVAL;
+	}
+
+	/* Handle ARCH_CET_ENABLE */
+	return -EINVAL;
+}