[PATCH RESEND v2 1/2] arch/x86: handle ARCH_SHSTK_ENABLE explicitly

[PATCH RESEND v2 1/2] arch/x86: handle ARCH_SHSTK_ENABLE explicitly

[Bill Roberts]

On x86 user shadow stack is controlled via an arch_prctl() interface, in
order to allow ptrace to actuate some of the options, and allow normal
syscall execution of arch_prctl() to execute others. Only valid shadow stack
options, for both ptrace and normal userspace callers, are directed into the
shstk_prctl() handler via checking in do_arch_prctl_64(). Then in
shstk_prctl(), it differentiates ptrace callers by checking if task !=
current.

The trouble starts when the handling of the task == current (normal syscall)
case treats the enabling option, ARCH_SHSTK_ENABLE, like a default case. It
reasons that shstk_prctl() has handled all other possible shadow stack
options, so ARCH_SHSTK_ENABLE can be the only remaining. Or so it thinks.
Because the ARCH_SHSTK_UNLOCK option is only handled in the ptrace case.
Since it has no handler in the syscall case, ARCH_SHSTK_UNLOCK can get
misinterpreted as ARCH_SHSTK_ENABLE by the erroneous default case logic.

The impact is that doing an arch_prtcl(ARCH_SHSTK_UNLOCK) syscall actually
behaves like an arch_prtcl(ARCH_SHSTK_ENABLE) syscall. This is a surprising
behavior, but does not expose any control to userspace that it doesn't
already have.

So drop the buggy default assumptions by checking for ARCH_SHSTK_ENABLE
explicitly.

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Bill Roberts <bill.roberts@arm.com>
---
 arch/x86/kernel/shstk.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
index eb690ba90180..8deed118040c 100644
--- a/arch/x86/kernel/shstk.c
+++ b/arch/x86/kernel/shstk.c
@@ -607,11 +607,13 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
 		return -EINVAL;
 	}
 
-	/* Handle ARCH_SHSTK_ENABLE */
-	if (features & ARCH_SHSTK_SHSTK)
-		return shstk_setup();
-	if (features & ARCH_SHSTK_WRSS)
-		return wrss_control(true);
+	if (option == ARCH_SHSTK_ENABLE) {
+		if (features & ARCH_SHSTK_SHSTK)
+			return shstk_setup();
+		if (features & ARCH_SHSTK_WRSS)
+			return wrss_control(true);
+	}
+
 	return -EINVAL;
 }
 
-- 
2.55.0

[PATCH RESEND v2 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Bill Roberts]

Test that ARCH_SHSTK_UNLOCK can't be called from normal (non-ptrace)
context.

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Bill Roberts <bill.roberts@arm.com>
---
 tools/testing/selftests/x86/test_shadow_stack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 3d6ca33edba4..cbe7ca4ebd90 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -1059,6 +1059,14 @@ int main(int argc, char *argv[])
 {
 	int ret = 0;
 
+	/* Test that ARCH_SHSTK_UNLOCK can't be called from normal (non-ptrace) context. */
+	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
+		printf("[FAIL]\tARCH_SHSTK_UNLOCK should fail in a non-ptrace context\n");
+		goto out;
+	}
+
+	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
+
 	if (ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)) {
 		printf("[SKIP]\tCould not enable Shadow stack\n");
 		return 1;
-- 
2.55.0