Re: [RFC PATCH v1 15/28] riscv/mm: Implement map_shadow_stack() syscall

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 25, 2024 at 01:24:16PM -0800, Charlie Jenkins wrote:
On Wed, Jan 24, 2024 at 10:21:40PM -0800, debug@xxxxxxxxxxxx wrote:
From: Deepak Gupta <debug@xxxxxxxxxxxx>

As discussed extensively in the changelog for the addition of this
syscall on x86 ("x86/shstk: Introduce map_shadow_stack syscall") the
existing mmap() and madvise() syscalls do not map entirely well onto the
security requirements for guarded control stacks since they lead to
windows where memory is allocated but not yet protected or stacks which
are not properly and safely initialised. Instead a new syscall
map_shadow_stack() has been defined which allocates and initialises a
shadow stack page.

This patch implements this syscall for riscv. riscv doesn't require token
to be setup by kernel because user mode can do that by itself. However to
provide compatiblity and portability with other architectues, user mode can
specify token set flag.

Signed-off-by: Deepak Gupta <debug@xxxxxxxxxxxx>
---
 arch/riscv/kernel/Makefile      |   2 +
 arch/riscv/kernel/usercfi.c     | 150 ++++++++++++++++++++++++++++++++
 include/uapi/asm-generic/mman.h |   1 +
 3 files changed, 153 insertions(+)
 create mode 100644 arch/riscv/kernel/usercfi.c

diff --git a/arch/riscv/kernel/Makefile b/arch/riscv/kernel/Makefile
index fee22a3d1b53..8c668269e886 100644
--- a/arch/riscv/kernel/Makefile
+++ b/arch/riscv/kernel/Makefile
@@ -102,3 +102,5 @@ obj-$(CONFIG_COMPAT)		+= compat_vdso/

 obj-$(CONFIG_64BIT)		+= pi/
 obj-$(CONFIG_ACPI)		+= acpi.o
+
+obj-$(CONFIG_RISCV_USER_CFI) += usercfi.o
diff --git a/arch/riscv/kernel/usercfi.c b/arch/riscv/kernel/usercfi.c
new file mode 100644
index 000000000000..35ede2cbc05b
--- /dev/null
+++ b/arch/riscv/kernel/usercfi.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 Rivos, Inc.
Nit: Should be updated to 2024

noted

+ * Deepak Gupta <debug@xxxxxxxxxxxx>
+ */
+
+#include <linux/sched.h>
+#include <linux/bitops.h>
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/mman.h>
+#include <linux/uaccess.h>
+#include <linux/sizes.h>
+#include <linux/user.h>
+#include <linux/syscalls.h>
+#include <linux/prctl.h>
+#include <asm/csr.h>
+#include <asm/usercfi.h>
+
+#define SHSTK_ENTRY_SIZE sizeof(void *)
+
+/*
+ * Writes on shadow stack can either be `sspush` or `ssamoswap`. `sspush` can happen
+ * implicitly on current shadow stack pointed to by CSR_SSP. `ssamoswap` takes pointer to
+ * shadow stack. To keep it simple, we plan to use `ssamoswap` to perform writes on shadow
+ * stack.
+ */
+static noinline unsigned long amo_user_shstk(unsigned long *addr, unsigned long val)
+{
+	/*
+	 * In case ssamoswap faults, return -1.
+	 * Never expect -1 on shadow stack. Expect return addresses and zero
+	 */
+	unsigned long swap = -1;
+
+	__enable_user_access();
+	asm_volatile_goto(
+				".option push\n"
+				".option arch, +zicfiss\n"
+#ifdef CONFIG_64BIT
+				"1: ssamoswap.d %0, %2, %1\n"
+#else
+				"1: ssamoswap.w %0, %2, %1\n"

A SSAMOSWAP macro that conditionally defines this would be cleaner

Yes I need to do that. Infact I need to gate CONFIG_RISCV_USER_CFI behind
some riscv-gnu toolchain version as well. Becuase not all toolchain versions will
recognize this.


+#endif
+				_ASM_EXTABLE(1b, %l[fault])
+				RISCV_ACQUIRE_BARRIER
+				".option pop\n"
+				: "=r" (swap), "+A" (*addr)

I just ran into this on one of my patches that not every compiler
supports output args in asm goto blocks. You need to guard this with the
kconfig option CC_HAS_ASM_GOTO_TIED_OUTPUT. Unfortunately, that means
that this code needs two versions, or you can choose to gate CFI behind
this option, it's supported by recent versions of GCC/CLANG.

Thanks.
I'll gate behind CC_HAS_ASM_GOTO_TIED_OUTPUT. Earlier versions of GCC/CLANG
won't have CFI support in them anyways.


For readability it is also nice to use labels for the asm variables such
as `"=r" (swap)` can be `[swap] "=r" (swap)` and then replace %0 with
%[swap].

noted, will do that. I copied it from gcc asm snippet `amoswap` somewhere in
kernel. Goes without saying, I am terrible with gcc asm syntax.


- Charlie

+				: "r" (val)
+				: "memory"
+				: fault




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux