[PATCH v3 0/2] AARCH64: Enable GCC-based Shadow Call Stack

[PATCH v3 0/2] AARCH64: Enable GCC-based Shadow Call Stack

[Dan Li]

Shadow call stacks will be available in GCC >= 12, this series makes
the corresponding kernel configuration available when compiling
the kernel with the gcc and adds corresponding tests in lkdtm.

Dan Li (2):
  AARCH64: Add gcc Shadow Call Stack support
  lkdtm: Add Shadow Call Stack tests

 arch/Kconfig                            | 19 +++----
 arch/arm64/Kconfig                      |  2 +-
 drivers/misc/lkdtm/Makefile             |  1 +
 drivers/misc/lkdtm/core.c               |  2 +
 drivers/misc/lkdtm/lkdtm.h              |  4 ++
 drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
 include/linux/compiler-gcc.h            |  4 ++
 tools/testing/selftests/lkdtm/tests.txt |  2 +
 8 files changed, 91 insertions(+), 10 deletions(-)
 create mode 100644 drivers/misc/lkdtm/scs.c

-- 
2.17.1

[PATCH v3 0/2] AARCH64: Enable GCC-based Shadow Call Stack

[Dan Li]

Shadow call stacks will be available in GCC >= 12, this series makes
the corresponding kernel configuration available when compiling
the kernel with the gcc and adds corresponding tests in lkdtm.

Dan Li (2):
  AARCH64: Add gcc Shadow Call Stack support
  lkdtm: Add Shadow Call Stack tests

 arch/Kconfig                            | 19 +++----
 arch/arm64/Kconfig                      |  2 +-
 drivers/misc/lkdtm/Makefile             |  1 +
 drivers/misc/lkdtm/core.c               |  2 +
 drivers/misc/lkdtm/lkdtm.h              |  4 ++
 drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
 include/linux/compiler-gcc.h            |  4 ++
 tools/testing/selftests/lkdtm/tests.txt |  2 +
 8 files changed, 91 insertions(+), 10 deletions(-)
 create mode 100644 drivers/misc/lkdtm/scs.c

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/2] AARCH64: Add gcc Shadow Call Stack support

[Dan Li]

Shadow call stacks will be available in GCC >= 12, this patch makes
the corresponding kernel configuration available when compiling
the kernel with the gcc.

Note that the implementation in GCC is slightly different from Clang.
With SCS enabled, functions will only pop x30 once in the epilogue,
like:

   str     x30, [x18], #8
   stp     x29, x30, [sp, #-16]!
   ......
-  ldp     x29, x30, [sp], #16	  //clang
+  ldr     x29, [sp], #16	  //GCC
   ldr     x30, [x18, #-8]!

Link: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=ce09ab17ddd21f73ff2caf6eec3b0ee9b0e1a11e

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 arch/Kconfig                 | 19 ++++++++++---------
 arch/arm64/Kconfig           |  2 +-
 include/linux/compiler-gcc.h |  4 ++++
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 678a80713b21..cbbe824fe8b2 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -599,21 +599,22 @@ config STACKPROTECTOR_STRONG
 config ARCH_SUPPORTS_SHADOW_CALL_STACK
 	bool
 	help
-	  An architecture should select this if it supports Clang's Shadow
-	  Call Stack and implements runtime support for shadow stack
+	  An architecture should select this if it supports the compiler's
+	  Shadow Call Stack and implements runtime support for shadow stack
 	  switching.
 
 config SHADOW_CALL_STACK
-	bool "Clang Shadow Call Stack"
-	depends on CC_IS_CLANG && ARCH_SUPPORTS_SHADOW_CALL_STACK
+	bool "Shadow Call Stack"
+	depends on ARCH_SUPPORTS_SHADOW_CALL_STACK
 	depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER
 	help
-	  This option enables Clang's Shadow Call Stack, which uses a
-	  shadow stack to protect function return addresses from being
-	  overwritten by an attacker. More information can be found in
-	  Clang's documentation:
+	  This option enables the compiler's Shadow Call Stack, which
+	  uses a shadow stack to protect function return addresses from
+	  being overwritten by an attacker. More information can be found
+	  in the compiler's documentation:
 
-	    https://clang.llvm.org/docs/ShadowCallStack.html
+	  - Clang: https://clang.llvm.org/docs/ShadowCallStack.html
+	  - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options
 
 	  Note that security guarantees in the kernel differ from the
 	  ones documented for user space. The kernel must store addresses
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 09b885cc4db5..b7145337efae 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1255,7 +1255,7 @@ config HW_PERF_EVENTS
 config ARCH_HAS_FILTER_PGPROT
 	def_bool y
 
-# Supported by clang >= 7.0
+# Supported by clang >= 7.0 or GCC >= 12.0.0
 config CC_HAVE_SHADOW_CALL_STACK
 	def_bool $(cc-option, -fsanitize=shadow-call-stack -ffixed-x18)
 
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index ccbbd31b3aae..deff5b308470 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -97,6 +97,10 @@
 #define KASAN_ABI_VERSION 4
 #endif
 
+#ifdef CONFIG_SHADOW_CALL_STACK
+#define __noscs __attribute__((__no_sanitize__("shadow-call-stack")))
+#endif
+
 #if __has_attribute(__no_sanitize_address__)
 #define __no_sanitize_address __attribute__((no_sanitize_address))
 #else
-- 
2.17.1

[PATCH v3 1/2] AARCH64: Add gcc Shadow Call Stack support

[Dan Li]

Shadow call stacks will be available in GCC >= 12, this patch makes
the corresponding kernel configuration available when compiling
the kernel with the gcc.

Note that the implementation in GCC is slightly different from Clang.
With SCS enabled, functions will only pop x30 once in the epilogue,
like:

   str     x30, [x18], #8
   stp     x29, x30, [sp, #-16]!
   ......
-  ldp     x29, x30, [sp], #16	  //clang
+  ldr     x29, [sp], #16	  //GCC
   ldr     x30, [x18, #-8]!

Link: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=ce09ab17ddd21f73ff2caf6eec3b0ee9b0e1a11e

Reviewed-by: Nathan Chancellor <nathan@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 arch/Kconfig                 | 19 ++++++++++---------
 arch/arm64/Kconfig           |  2 +-
 include/linux/compiler-gcc.h |  4 ++++
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/Kconfig b/arch/Kconfig
index 678a80713b21..cbbe824fe8b2 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -599,21 +599,22 @@ config STACKPROTECTOR_STRONG
 config ARCH_SUPPORTS_SHADOW_CALL_STACK
 	bool
 	help
-	  An architecture should select this if it supports Clang's Shadow
-	  Call Stack and implements runtime support for shadow stack
+	  An architecture should select this if it supports the compiler's
+	  Shadow Call Stack and implements runtime support for shadow stack
 	  switching.
 
 config SHADOW_CALL_STACK
-	bool "Clang Shadow Call Stack"
-	depends on CC_IS_CLANG && ARCH_SUPPORTS_SHADOW_CALL_STACK
+	bool "Shadow Call Stack"
+	depends on ARCH_SUPPORTS_SHADOW_CALL_STACK
 	depends on DYNAMIC_FTRACE_WITH_REGS || !FUNCTION_GRAPH_TRACER
 	help
-	  This option enables Clang's Shadow Call Stack, which uses a
-	  shadow stack to protect function return addresses from being
-	  overwritten by an attacker. More information can be found in
-	  Clang's documentation:
+	  This option enables the compiler's Shadow Call Stack, which
+	  uses a shadow stack to protect function return addresses from
+	  being overwritten by an attacker. More information can be found
+	  in the compiler's documentation:
 
-	    https://clang.llvm.org/docs/ShadowCallStack.html
+	  - Clang: https://clang.llvm.org/docs/ShadowCallStack.html
+	  - GCC: https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#Instrumentation-Options
 
 	  Note that security guarantees in the kernel differ from the
 	  ones documented for user space. The kernel must store addresses
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 09b885cc4db5..b7145337efae 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1255,7 +1255,7 @@ config HW_PERF_EVENTS
 config ARCH_HAS_FILTER_PGPROT
 	def_bool y
 
-# Supported by clang >= 7.0
+# Supported by clang >= 7.0 or GCC >= 12.0.0
 config CC_HAVE_SHADOW_CALL_STACK
 	def_bool $(cc-option, -fsanitize=shadow-call-stack -ffixed-x18)
 
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index ccbbd31b3aae..deff5b308470 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -97,6 +97,10 @@
 #define KASAN_ABI_VERSION 4
 #endif
 
+#ifdef CONFIG_SHADOW_CALL_STACK
+#define __noscs __attribute__((__no_sanitize__("shadow-call-stack")))
+#endif
+
 #if __has_attribute(__no_sanitize_address__)
 #define __no_sanitize_address __attribute__((no_sanitize_address))
 #else
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v3 1/2] AARCH64: Add gcc Shadow Call Stack support

[Kees Cook]

On Wed, 2 Mar 2022 23:43:23 -0800, Dan Li wrote:
> Shadow call stacks will be available in GCC >= 12, this patch makes
> the corresponding kernel configuration available when compiling
> the kernel with the gcc.
> 
> Note that the implementation in GCC is slightly different from Clang.
> With SCS enabled, functions will only pop x30 once in the epilogue,
> like:
> 
> [...]

I'm taking this one now so it'll make the merge window. We can hammer
out the lkdtm test after that.

Applied to for-next/hardening, thanks!

[1/2] arm64: Add gcc Shadow Call Stack support
      https://git.kernel.org/kees/c/afcf5441b9ff

-- 
Kees Cook

Re: (subset) [PATCH v3 1/2] AARCH64: Add gcc Shadow Call Stack support

[Kees Cook]

On Wed, 2 Mar 2022 23:43:23 -0800, Dan Li wrote:
> Shadow call stacks will be available in GCC >= 12, this patch makes
> the corresponding kernel configuration available when compiling
> the kernel with the gcc.
> 
> Note that the implementation in GCC is slightly different from Clang.
> With SCS enabled, functions will only pop x30 once in the epilogue,
> like:
> 
> [...]

I'm taking this one now so it'll make the merge window. We can hammer
out the lkdtm test after that.

Applied to for-next/hardening, thanks!

[1/2] arm64: Add gcc Shadow Call Stack support
      https://git.kernel.org/kees/c/afcf5441b9ff

-- 
Kees Cook


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Add tests for SCS (Shadow Call Stack) based
backward CFI (as implemented by Clang and GCC).

Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 drivers/misc/lkdtm/Makefile             |  1 +
 drivers/misc/lkdtm/core.c               |  2 +
 drivers/misc/lkdtm/lkdtm.h              |  4 ++
 drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |  2 +
 5 files changed, 76 insertions(+)
 create mode 100644 drivers/misc/lkdtm/scs.c

diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
index 2e0aa74ac185..e2fb17868af2 100644
--- a/drivers/misc/lkdtm/Makefile
+++ b/drivers/misc/lkdtm/Makefile
@@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM)		+= rodata_objcopy.o
 lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
 lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
 lkdtm-$(CONFIG_LKDTM)		+= cfi.o
+lkdtm-$(CONFIG_LKDTM)		+= scs.o
 lkdtm-$(CONFIG_LKDTM)		+= fortify.o
 lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
 
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index f69b964b9952..d0ce0bec117c 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(STACKLEAK_ERASING),
 	CRASHTYPE(CFI_FORWARD_PROTO),
+	CRASHTYPE(CFI_BACKWARD_SHADOW),
+	CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS),
 	CRASHTYPE(FORTIFIED_OBJECT),
 	CRASHTYPE(FORTIFIED_SUBOBJECT),
 	CRASHTYPE(FORTIFIED_STRSCPY),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index d6137c70ebbe..a23d32dfc10b 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void);
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
 
+/* scs.c */
+void lkdtm_CFI_BACKWARD_SHADOW(void);
+void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void);
+
 /* fortify.c */
 void lkdtm_FORTIFIED_OBJECT(void);
 void lkdtm_FORTIFIED_SUBOBJECT(void);
diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c
new file mode 100644
index 000000000000..5922a55a8844
--- /dev/null
+++ b/drivers/misc/lkdtm/scs.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This is for all the tests relating directly to Shadow Call Stack.
+ */
+#include "lkdtm.h"
+
+#ifdef CONFIG_ARM64
+/* Function clears its return address. */
+static noinline void lkdtm_scs_clear_lr(void)
+{
+	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
+
+	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
+}
+
+/* Function with __noscs attribute clears its return address. */
+static noinline void __noscs lkdtm_noscs_clear_lr(void)
+{
+	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
+
+	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
+}
+#endif
+
+/*
+ * This tries to call a function protected by Shadow Call Stack,
+ * which corrupts its own return address during execution.
+ * Due to the protection, the corruption will not take effect
+ * when the function returns.
+ */
+void lkdtm_CFI_BACKWARD_SHADOW(void)
+{
+#ifdef CONFIG_ARM64
+	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
+		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
+		return;
+	}
+
+	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
+	lkdtm_scs_clear_lr();
+
+	pr_err("ok: scs takes effect.\n");
+#else
+	pr_err("XFAIL: this test is arm64-only\n");
+#endif
+}
+
+/*
+ * This tries to call a function not protected by Shadow Call Stack,
+ * which corrupts its own return address during execution.
+ */
+void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
+{
+#ifdef CONFIG_ARM64
+	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
+		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
+		return;
+	}
+
+	pr_info("Trying to corrupt lr in a function with attribute __noscs ...\n");
+	lkdtm_noscs_clear_lr();
+
+	pr_err("FAIL: __noscs attribute does not take effect!\n");
+#else
+	pr_err("XFAIL: this test is arm64-only\n");
+#endif
+}
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..c849765c8dcc 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -73,6 +73,8 @@ USERCOPY_STACK_BEYOND
 USERCOPY_KERNEL
 STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
 CFI_FORWARD_PROTO
+CFI_BACKWARD_SHADOW ok: scs takes effect
+CFI_BACKWARD_SHADOW_WITH_NOSCS
 FORTIFIED_STRSCPY
 FORTIFIED_OBJECT
 FORTIFIED_SUBOBJECT
-- 
2.17.1

[PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Add tests for SCS (Shadow Call Stack) based
backward CFI (as implemented by Clang and GCC).

Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 drivers/misc/lkdtm/Makefile             |  1 +
 drivers/misc/lkdtm/core.c               |  2 +
 drivers/misc/lkdtm/lkdtm.h              |  4 ++
 drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |  2 +
 5 files changed, 76 insertions(+)
 create mode 100644 drivers/misc/lkdtm/scs.c

diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
index 2e0aa74ac185..e2fb17868af2 100644
--- a/drivers/misc/lkdtm/Makefile
+++ b/drivers/misc/lkdtm/Makefile
@@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM)		+= rodata_objcopy.o
 lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
 lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
 lkdtm-$(CONFIG_LKDTM)		+= cfi.o
+lkdtm-$(CONFIG_LKDTM)		+= scs.o
 lkdtm-$(CONFIG_LKDTM)		+= fortify.o
 lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
 
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index f69b964b9952..d0ce0bec117c 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(STACKLEAK_ERASING),
 	CRASHTYPE(CFI_FORWARD_PROTO),
+	CRASHTYPE(CFI_BACKWARD_SHADOW),
+	CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS),
 	CRASHTYPE(FORTIFIED_OBJECT),
 	CRASHTYPE(FORTIFIED_SUBOBJECT),
 	CRASHTYPE(FORTIFIED_STRSCPY),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index d6137c70ebbe..a23d32dfc10b 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void);
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
 
+/* scs.c */
+void lkdtm_CFI_BACKWARD_SHADOW(void);
+void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void);
+
 /* fortify.c */
 void lkdtm_FORTIFIED_OBJECT(void);
 void lkdtm_FORTIFIED_SUBOBJECT(void);
diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c
new file mode 100644
index 000000000000..5922a55a8844
--- /dev/null
+++ b/drivers/misc/lkdtm/scs.c
@@ -0,0 +1,67 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This is for all the tests relating directly to Shadow Call Stack.
+ */
+#include "lkdtm.h"
+
+#ifdef CONFIG_ARM64
+/* Function clears its return address. */
+static noinline void lkdtm_scs_clear_lr(void)
+{
+	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
+
+	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
+}
+
+/* Function with __noscs attribute clears its return address. */
+static noinline void __noscs lkdtm_noscs_clear_lr(void)
+{
+	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
+
+	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
+}
+#endif
+
+/*
+ * This tries to call a function protected by Shadow Call Stack,
+ * which corrupts its own return address during execution.
+ * Due to the protection, the corruption will not take effect
+ * when the function returns.
+ */
+void lkdtm_CFI_BACKWARD_SHADOW(void)
+{
+#ifdef CONFIG_ARM64
+	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
+		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
+		return;
+	}
+
+	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
+	lkdtm_scs_clear_lr();
+
+	pr_err("ok: scs takes effect.\n");
+#else
+	pr_err("XFAIL: this test is arm64-only\n");
+#endif
+}
+
+/*
+ * This tries to call a function not protected by Shadow Call Stack,
+ * which corrupts its own return address during execution.
+ */
+void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
+{
+#ifdef CONFIG_ARM64
+	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
+		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
+		return;
+	}
+
+	pr_info("Trying to corrupt lr in a function with attribute __noscs ...\n");
+	lkdtm_noscs_clear_lr();
+
+	pr_err("FAIL: __noscs attribute does not take effect!\n");
+#else
+	pr_err("XFAIL: this test is arm64-only\n");
+#endif
+}
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..c849765c8dcc 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -73,6 +73,8 @@ USERCOPY_STACK_BEYOND
 USERCOPY_KERNEL
 STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
 CFI_FORWARD_PROTO
+CFI_BACKWARD_SHADOW ok: scs takes effect
+CFI_BACKWARD_SHADOW_WITH_NOSCS
 FORTIFIED_STRSCPY
 FORTIFIED_OBJECT
 FORTIFIED_SUBOBJECT
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based
> backward CFI (as implemented by Clang and GCC).

Cool; thanks for writing these!

> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>  drivers/misc/lkdtm/Makefile             |  1 +
>  drivers/misc/lkdtm/core.c               |  2 +
>  drivers/misc/lkdtm/lkdtm.h              |  4 ++
>  drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
>  tools/testing/selftests/lkdtm/tests.txt |  2 +
>  5 files changed, 76 insertions(+)
>  create mode 100644 drivers/misc/lkdtm/scs.c
> 
> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> index 2e0aa74ac185..e2fb17868af2 100644
> --- a/drivers/misc/lkdtm/Makefile
> +++ b/drivers/misc/lkdtm/Makefile
> @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM)		+= rodata_objcopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>  lkdtm-$(CONFIG_LKDTM)		+= cfi.o
> +lkdtm-$(CONFIG_LKDTM)		+= scs.o

I'd expect these to be in cfi.c, rather than making a new source file.

>  lkdtm-$(CONFIG_LKDTM)		+= fortify.o
>  lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
>  
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..d0ce0bec117c 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = {
>  	CRASHTYPE(USERCOPY_KERNEL),
>  	CRASHTYPE(STACKLEAK_ERASING),
>  	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS),
>  	CRASHTYPE(FORTIFIED_OBJECT),
>  	CRASHTYPE(FORTIFIED_SUBOBJECT),
>  	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a23d32dfc10b 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void);
>  /* cfi.c */
>  void lkdtm_CFI_FORWARD_PROTO(void);
>  
> +/* scs.c */
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void);
> +
>  /* fortify.c */
>  void lkdtm_FORTIFIED_OBJECT(void);
>  void lkdtm_FORTIFIED_SUBOBJECT(void);
> diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c
> new file mode 100644
> index 000000000000..5922a55a8844
> --- /dev/null
> +++ b/drivers/misc/lkdtm/scs.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This is for all the tests relating directly to Shadow Call Stack.
> + */
> +#include "lkdtm.h"
> +
> +#ifdef CONFIG_ARM64
> +/* Function clears its return address. */
> +static noinline void lkdtm_scs_clear_lr(void)
> +{
> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");

Is the asm needed here? Why not:

	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;

	*lr = 0;

> +}
> +
> +/* Function with __noscs attribute clears its return address. */
> +static noinline void __noscs lkdtm_noscs_clear_lr(void)
> +{
> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
> +}
> +#endif
> +
> +/*
> + * This tries to call a function protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + * Due to the protection, the corruption will not take effect
> + * when the function returns.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW(void)

I think these two tests should be collapsed into a single one.

> +{
> +#ifdef CONFIG_ARM64
> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> +		return;
> +	}
> +
> +	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +	lkdtm_scs_clear_lr();
> +
> +	pr_err("ok: scs takes effect.\n");
> +#else
> +	pr_err("XFAIL: this test is arm64-only\n");
> +#endif

This is slightly surprising -- we have no detection when a function has
its non-shadow-stack return address corrupted: it just _ignores_ the
value stored there. That seems like a missed opportunity for warning
about an unexpected state.

> +}
> +
> +/*
> + * This tries to call a function not protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
> +{
> +#ifdef CONFIG_ARM64
> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> +		return;

Other tests try to give some hints about failures, e.g.:

		pr_err("FAIL: cannot change for SCS\n");
		pr_expected_config(CONFIG_SHADOW_CALL_STACK);

Though, having the IS_ENABLED in there makes me wonder if this test
should instead be made _survivable_ on failure. Something like this,
completely untested:


#ifdef CONFIG_ARM64
static noinline void lkdtm_scs_set_lr(unsigned long *addr)
{
	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
	*lr = addr;
}

/* Function with __noscs attribute clears its return address. */
static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
{
	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
	*lr = addr;
}
#endif


void lkdtm_CFI_BACKWARD_SHADOW(void)
{
#ifdef CONFIG_ARM64

	/* Verify the "normal" condition of LR corruption working. */
	do {
		/* Keep label in scope to avoid compiler warning. */
		if ((volatile int)0)
			goto unexpected;

		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
		lkdtm_noscs_set_lr(&&expected);

unexpected:
		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
		break;

expected:
		pr_err("ok: lr corruption redirected without scs.\n");
	} while (0);


	do {
		/* Keep labe in scope to avoid compiler warning. */
		if ((volatile int)0)
			goto good_scs;

		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
		lkdtm_scs_set_lr(&&bad_scs);

good_scs:
		pr_info("ok: scs takes effect.\n");
		break;

bad_scs:
		pr_err("FAIL: return address rewritten!\n");
		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
	} while (0);
#else
	pr_err("XFAIL: this test is arm64-only\n");
#endif
}

And we should, actually, be able to make the "set_lr" functions be
arch-specific, leaving the test itself arch-agnostic....

-- 
Kees Cook

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based
> backward CFI (as implemented by Clang and GCC).

Cool; thanks for writing these!

> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>  drivers/misc/lkdtm/Makefile             |  1 +
>  drivers/misc/lkdtm/core.c               |  2 +
>  drivers/misc/lkdtm/lkdtm.h              |  4 ++
>  drivers/misc/lkdtm/scs.c                | 67 +++++++++++++++++++++++++
>  tools/testing/selftests/lkdtm/tests.txt |  2 +
>  5 files changed, 76 insertions(+)
>  create mode 100644 drivers/misc/lkdtm/scs.c
> 
> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> index 2e0aa74ac185..e2fb17868af2 100644
> --- a/drivers/misc/lkdtm/Makefile
> +++ b/drivers/misc/lkdtm/Makefile
> @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM)		+= rodata_objcopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= usercopy.o
>  lkdtm-$(CONFIG_LKDTM)		+= stackleak.o
>  lkdtm-$(CONFIG_LKDTM)		+= cfi.o
> +lkdtm-$(CONFIG_LKDTM)		+= scs.o

I'd expect these to be in cfi.c, rather than making a new source file.

>  lkdtm-$(CONFIG_LKDTM)		+= fortify.o
>  lkdtm-$(CONFIG_PPC_64S_HASH_MMU)	+= powerpc.o
>  
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..d0ce0bec117c 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = {
>  	CRASHTYPE(USERCOPY_KERNEL),
>  	CRASHTYPE(STACKLEAK_ERASING),
>  	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS),
>  	CRASHTYPE(FORTIFIED_OBJECT),
>  	CRASHTYPE(FORTIFIED_SUBOBJECT),
>  	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a23d32dfc10b 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void);
>  /* cfi.c */
>  void lkdtm_CFI_FORWARD_PROTO(void);
>  
> +/* scs.c */
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void);
> +
>  /* fortify.c */
>  void lkdtm_FORTIFIED_OBJECT(void);
>  void lkdtm_FORTIFIED_SUBOBJECT(void);
> diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c
> new file mode 100644
> index 000000000000..5922a55a8844
> --- /dev/null
> +++ b/drivers/misc/lkdtm/scs.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This is for all the tests relating directly to Shadow Call Stack.
> + */
> +#include "lkdtm.h"
> +
> +#ifdef CONFIG_ARM64
> +/* Function clears its return address. */
> +static noinline void lkdtm_scs_clear_lr(void)
> +{
> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");

Is the asm needed here? Why not:

	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;

	*lr = 0;

> +}
> +
> +/* Function with __noscs attribute clears its return address. */
> +static noinline void __noscs lkdtm_noscs_clear_lr(void)
> +{
> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
> +}
> +#endif
> +
> +/*
> + * This tries to call a function protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + * Due to the protection, the corruption will not take effect
> + * when the function returns.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW(void)

I think these two tests should be collapsed into a single one.

> +{
> +#ifdef CONFIG_ARM64
> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> +		return;
> +	}
> +
> +	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +	lkdtm_scs_clear_lr();
> +
> +	pr_err("ok: scs takes effect.\n");
> +#else
> +	pr_err("XFAIL: this test is arm64-only\n");
> +#endif

This is slightly surprising -- we have no detection when a function has
its non-shadow-stack return address corrupted: it just _ignores_ the
value stored there. That seems like a missed opportunity for warning
about an unexpected state.

> +}
> +
> +/*
> + * This tries to call a function not protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
> +{
> +#ifdef CONFIG_ARM64
> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> +		return;

Other tests try to give some hints about failures, e.g.:

		pr_err("FAIL: cannot change for SCS\n");
		pr_expected_config(CONFIG_SHADOW_CALL_STACK);

Though, having the IS_ENABLED in there makes me wonder if this test
should instead be made _survivable_ on failure. Something like this,
completely untested:


#ifdef CONFIG_ARM64
static noinline void lkdtm_scs_set_lr(unsigned long *addr)
{
	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
	*lr = addr;
}

/* Function with __noscs attribute clears its return address. */
static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
{
	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
	*lr = addr;
}
#endif


void lkdtm_CFI_BACKWARD_SHADOW(void)
{
#ifdef CONFIG_ARM64

	/* Verify the "normal" condition of LR corruption working. */
	do {
		/* Keep label in scope to avoid compiler warning. */
		if ((volatile int)0)
			goto unexpected;

		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
		lkdtm_noscs_set_lr(&&expected);

unexpected:
		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
		break;

expected:
		pr_err("ok: lr corruption redirected without scs.\n");
	} while (0);


	do {
		/* Keep labe in scope to avoid compiler warning. */
		if ((volatile int)0)
			goto good_scs;

		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
		lkdtm_scs_set_lr(&&bad_scs);

good_scs:
		pr_info("ok: scs takes effect.\n");
		break;

bad_scs:
		pr_err("FAIL: return address rewritten!\n");
		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
	} while (0);
#else
	pr_err("XFAIL: this test is arm64-only\n");
#endif
}

And we should, actually, be able to make the "set_lr" functions be
arch-specific, leaving the test itself arch-agnostic....

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
> Though, having the IS_ENABLED in there makes me wonder if this test
> should instead be made _survivable_ on failure. Something like this,
> completely untested:
> 
> 
> #ifdef CONFIG_ARM64
> static noinline void lkdtm_scs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> 
> /* Function with __noscs attribute clears its return address. */
> static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> #endif
> 
> 
> void lkdtm_CFI_BACKWARD_SHADOW(void)
> {
> #ifdef CONFIG_ARM64
> 
> 	/* Verify the "normal" condition of LR corruption working. */
> 	do {
> 		/* Keep label in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto unexpected;
> 
> 		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> 		lkdtm_noscs_set_lr(&&expected);
> 
> unexpected:
> 		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> 		break;
> 
> expected:
> 		pr_err("ok: lr corruption redirected without scs.\n");
> 	} while (0);
> 
> 
> 	do {
> 		/* Keep labe in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto good_scs;
> 
> 		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> 		lkdtm_scs_set_lr(&&bad_scs);
> 
> good_scs:
> 		pr_info("ok: scs takes effect.\n");
> 		break;
> 
> bad_scs:
> 		pr_err("FAIL: return address rewritten!\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 	} while (0);
> #else
> 	pr_err("XFAIL: this test is arm64-only\n");
> #endif
> }
> 
> And we should, actually, be able to make the "set_lr" functions be
> arch-specific, leaving the test itself arch-agnostic....

Yeah, as a tested example, this works for x86_64, and based on what you
had, I'd expect it to work on arm64 too:

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
    /* Use of volatile is to make sure final write isn't seen as a dead store. */
    unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

    /* Make sure we've found the right place on the stack before writing it. */
    if (*ret_addr == expected)
        *ret_addr = addr;
}

volatile int force_label;
int main(void)
{
    do {
        /* Keep labels in scope. */
        if (force_label)
            goto normal;
        if (force_label)
            goto redirected;

        set_return_addr(&&normal, &&redirected);
normal:
        printf("I should be skipped\n");
        break;
redirected:
        printf("Redirected\n");
    } while (0);

    return 0;
}


It does _not_ work under Clang, though, which I'm still looking at.

-- 
Kees Cook

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
> Though, having the IS_ENABLED in there makes me wonder if this test
> should instead be made _survivable_ on failure. Something like this,
> completely untested:
> 
> 
> #ifdef CONFIG_ARM64
> static noinline void lkdtm_scs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> 
> /* Function with __noscs attribute clears its return address. */
> static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> #endif
> 
> 
> void lkdtm_CFI_BACKWARD_SHADOW(void)
> {
> #ifdef CONFIG_ARM64
> 
> 	/* Verify the "normal" condition of LR corruption working. */
> 	do {
> 		/* Keep label in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto unexpected;
> 
> 		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> 		lkdtm_noscs_set_lr(&&expected);
> 
> unexpected:
> 		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> 		break;
> 
> expected:
> 		pr_err("ok: lr corruption redirected without scs.\n");
> 	} while (0);
> 
> 
> 	do {
> 		/* Keep labe in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto good_scs;
> 
> 		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> 		lkdtm_scs_set_lr(&&bad_scs);
> 
> good_scs:
> 		pr_info("ok: scs takes effect.\n");
> 		break;
> 
> bad_scs:
> 		pr_err("FAIL: return address rewritten!\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 	} while (0);
> #else
> 	pr_err("XFAIL: this test is arm64-only\n");
> #endif
> }
> 
> And we should, actually, be able to make the "set_lr" functions be
> arch-specific, leaving the test itself arch-agnostic....

Yeah, as a tested example, this works for x86_64, and based on what you
had, I'd expect it to work on arm64 too:

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
    /* Use of volatile is to make sure final write isn't seen as a dead store. */
    unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

    /* Make sure we've found the right place on the stack before writing it. */
    if (*ret_addr == expected)
        *ret_addr = addr;
}

volatile int force_label;
int main(void)
{
    do {
        /* Keep labels in scope. */
        if (force_label)
            goto normal;
        if (force_label)
            goto redirected;

        set_return_addr(&&normal, &&redirected);
normal:
        printf("I should be skipped\n");
        break;
redirected:
        printf("Redirected\n");
    } while (0);

    return 0;
}


It does _not_ work under Clang, though, which I'm still looking at.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 11:09, Kees Cook wrote:
> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>> Though, having the IS_ENABLED in there makes me wonder if this test
>> should instead be made _survivable_ on failure. Something like this,
>> completely untested:
>>
>>
>> And we should, actually, be able to make the "set_lr" functions be
>> arch-specific, leaving the test itself arch-agnostic....
> 
> Yeah, as a tested example, this works for x86_64, and based on what you
> had, I'd expect it to work on arm64 too:
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
>      if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> 
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;

 From the assembly code, it seems that "&&normal" does't always equal
to the address of label "normal" when we use clang with -O2.

> redirected:
>          printf("Redirected\n");
>      } while (0);
>

The address of "&&redirected" may appear in the middle of the assembly
instructions of the printf. If we unconditionally jump to "&&normal",
it may crash directly because x0 is not set correctly.

>      return 0;
> }
> 
> 
> It does _not_ work under Clang, though, which I'm still looking at.
> 

AFAICT, maybe we could specify -O0 optimization to bypass this.


BTW:
Occasionally found, the following code works correctly, but i think
it doesn't solve the issue :)

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
     /* Use of volatile is to make sure final write isn't seen as a dead store. */
     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

     /* Make sure we've found the right place on the stack before writing it. */
//    if (*ret_addr == expected)
         *ret_addr = addr;
}
volatile int force_label;
int main(void)
{
     do {
         /* Keep labels in scope. */
         if (force_label)
             goto normal;
         if (force_label)
             goto redirected;

         set_return_addr(&&normal, &&redirected);
normal:
         printf("I should be skipped\n");
         break;

redirected:
         printf("Redirected\n");
         printf("\n");				//add a new printf
     } while (0);

     return 0;
}

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 11:09, Kees Cook wrote:
> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>> Though, having the IS_ENABLED in there makes me wonder if this test
>> should instead be made _survivable_ on failure. Something like this,
>> completely untested:
>>
>>
>> And we should, actually, be able to make the "set_lr" functions be
>> arch-specific, leaving the test itself arch-agnostic....
> 
> Yeah, as a tested example, this works for x86_64, and based on what you
> had, I'd expect it to work on arm64 too:
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
>      if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> 
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;

 From the assembly code, it seems that "&&normal" does't always equal
to the address of label "normal" when we use clang with -O2.

> redirected:
>          printf("Redirected\n");
>      } while (0);
>

The address of "&&redirected" may appear in the middle of the assembly
instructions of the printf. If we unconditionally jump to "&&normal",
it may crash directly because x0 is not set correctly.

>      return 0;
> }
> 
> 
> It does _not_ work under Clang, though, which I'm still looking at.
> 

AFAICT, maybe we could specify -O0 optimization to bypass this.


BTW:
Occasionally found, the following code works correctly, but i think
it doesn't solve the issue :)

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
     /* Use of volatile is to make sure final write isn't seen as a dead store. */
     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

     /* Make sure we've found the right place on the stack before writing it. */
//    if (*ret_addr == expected)
         *ret_addr = addr;
}
volatile int force_label;
int main(void)
{
     do {
         /* Keep labels in scope. */
         if (force_label)
             goto normal;
         if (force_label)
             goto redirected;

         set_return_addr(&&normal, &&redirected);
normal:
         printf("I should be skipped\n");
         break;

redirected:
         printf("Redirected\n");
         printf("\n");				//add a new printf
     } while (0);

     return 0;
}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/4/22 06:54, Dan Li wrote:
> 
> 
> On 3/3/22 11:09, Kees Cook wrote:
>> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>>> Though, having the IS_ENABLED in there makes me wonder if this test
>>> should instead be made _survivable_ on failure. Something like this,
>>> completely untested:
>>>
>>>
>>> And we should, actually, be able to make the "set_lr" functions be
>>> arch-specific, leaving the test itself arch-agnostic....
>>
>> Yeah, as a tested example, this works for x86_64, and based on what you
>> had, I'd expect it to work on arm64 too:
>>
>> #include <stdio.h>
>>
>> static __attribute__((noinline))
>> void set_return_addr(unsigned long *expected, unsigned long *addr)
>> {
>>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
>>
>>      /* Make sure we've found the right place on the stack before writing it. */
>>      if (*ret_addr == expected)
>>          *ret_addr = addr;
>> }
>>
>> volatile int force_label;
>> int main(void)
>> {
>>      do {
>>          /* Keep labels in scope. */
>>          if (force_label)
>>              goto normal;
>>          if (force_label)
>>              goto redirected;
>>
>>          set_return_addr(&&normal, &&redirected);
>> normal:
>>          printf("I should be skipped\n");
>>          break;
> 
>  From the assembly code, it seems that "&&normal" does't always equal
> to the address of label "normal" when we use clang with -O2.
> 
>> redirected:
>>          printf("Redirected\n");
>>      } while (0);
>>
> 
> The address of "&&redirected" may appear in the middle of the assembly
> instructions of the printf. If we unconditionally jump to "&&normal",> it may crash directly because x0 is not set correctly.

Sorry, it should be:
The address of "&&redirected" may appear in the middle of the assembly
instructions of the printf. If we unconditionally jump to "&&redirected",
it may crash directly because x0 of printf is not set correctly.

Thanks,
Dan.
> 
>>      return 0;
>> }
>>
>>
>> It does _not_ work under Clang, though, which I'm still looking at.
>>
> 
> AFAICT, maybe we could specify -O0 optimization to bypass this.
> 
> 
> BTW:
> Occasionally found, the following code works correctly, but i think
> it doesn't solve the issue :)
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
> //    if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;
> 
> redirected:
>          printf("Redirected\n");
>          printf("\n");                //add a new printf
>      } while (0);
> 
>      return 0;
> }

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/4/22 06:54, Dan Li wrote:
> 
> 
> On 3/3/22 11:09, Kees Cook wrote:
>> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>>> Though, having the IS_ENABLED in there makes me wonder if this test
>>> should instead be made _survivable_ on failure. Something like this,
>>> completely untested:
>>>
>>>
>>> And we should, actually, be able to make the "set_lr" functions be
>>> arch-specific, leaving the test itself arch-agnostic....
>>
>> Yeah, as a tested example, this works for x86_64, and based on what you
>> had, I'd expect it to work on arm64 too:
>>
>> #include <stdio.h>
>>
>> static __attribute__((noinline))
>> void set_return_addr(unsigned long *expected, unsigned long *addr)
>> {
>>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
>>
>>      /* Make sure we've found the right place on the stack before writing it. */
>>      if (*ret_addr == expected)
>>          *ret_addr = addr;
>> }
>>
>> volatile int force_label;
>> int main(void)
>> {
>>      do {
>>          /* Keep labels in scope. */
>>          if (force_label)
>>              goto normal;
>>          if (force_label)
>>              goto redirected;
>>
>>          set_return_addr(&&normal, &&redirected);
>> normal:
>>          printf("I should be skipped\n");
>>          break;
> 
>  From the assembly code, it seems that "&&normal" does't always equal
> to the address of label "normal" when we use clang with -O2.
> 
>> redirected:
>>          printf("Redirected\n");
>>      } while (0);
>>
> 
> The address of "&&redirected" may appear in the middle of the assembly
> instructions of the printf. If we unconditionally jump to "&&normal",> it may crash directly because x0 is not set correctly.

Sorry, it should be:
The address of "&&redirected" may appear in the middle of the assembly
instructions of the printf. If we unconditionally jump to "&&redirected",
it may crash directly because x0 of printf is not set correctly.

Thanks,
Dan.
> 
>>      return 0;
>> }
>>
>>
>> It does _not_ work under Clang, though, which I'm still looking at.
>>
> 
> AFAICT, maybe we could specify -O0 optimization to bypass this.
> 
> 
> BTW:
> Occasionally found, the following code works correctly, but i think
> it doesn't solve the issue :)
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
> //    if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;
> 
> redirected:
>          printf("Redirected\n");
>          printf("\n");                //add a new printf
>      } while (0);
> 
>      return 0;
> }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 11:09, Kees Cook wrote:
> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>> And we should, actually, be able to make the "set_lr" functions be
>> arch-specific, leaving the test itself arch-agnostic....
> 
> Yeah, as a tested example, this works for x86_64, and based on what you
> had, I'd expect it to work on arm64 too:
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
>      if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> 
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;
> redirected:
>          printf("Redirected\n");
>      } while (0);
> 
>      return 0;
> }
> 
> 
> It does _not_ work under Clang, though, which I'm still looking at.
> 

The following code seems to work fine under clang/gcc, x86_64/aarch64
(also tested in lkdtm_CFI_BACKWARD_SHADOW):

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
     /* Use of volatile is to make sure final write isn't seen as a dead store. */
     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

     /* Make sure we've found the right place on the stack before writing it. */
     if(*ret_addr == expected)
         *ret_addr = (addr);
}

static volatile int force_label;

int main(void)
{
     void *array[] = {0, &&normal, &&redirected};

     if (force_label) {
         /* Call it with a NULL to avoid parameters being treated as constants in -02. */
         set_return_addr(NULL, NULL);
         goto * array[force_label];
     }

     do {

         set_return_addr(&&normal, &&redirected);

normal:
         printf("I should be skipped\n");
         break;

redirected:
         printf("Redirected\n");

     } while (0);

     return 0;
}

But currently it still crashes when I try to enable
"-mbranch-protection=pac-ret+leaf+bti".

Because the address of "&&redirected" is not encrypted under pac,
the autiasp check will fail when set_return_addr returns, and
eventually cause the function to crash when it returns to "&&redirected"
("&&redirected" as a reserved label always seems to start with a bti j
insn).

For lkdtm, if we're going to handle both cases in one function, maybe
it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
and maybe also turn off -O2 options for the function :)

Thanks,
Dan.

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 11:09, Kees Cook wrote:
> On Thu, Mar 03, 2022 at 10:42:45AM -0800, Kees Cook wrote:
>> And we should, actually, be able to make the "set_lr" functions be
>> arch-specific, leaving the test itself arch-agnostic....
> 
> Yeah, as a tested example, this works for x86_64, and based on what you
> had, I'd expect it to work on arm64 too:
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>      /* Use of volatile is to make sure final write isn't seen as a dead store. */
>      unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>      /* Make sure we've found the right place on the stack before writing it. */
>      if (*ret_addr == expected)
>          *ret_addr = addr;
> }
> 
> volatile int force_label;
> int main(void)
> {
>      do {
>          /* Keep labels in scope. */
>          if (force_label)
>              goto normal;
>          if (force_label)
>              goto redirected;
> 
>          set_return_addr(&&normal, &&redirected);
> normal:
>          printf("I should be skipped\n");
>          break;
> redirected:
>          printf("Redirected\n");
>      } while (0);
> 
>      return 0;
> }
> 
> 
> It does _not_ work under Clang, though, which I'm still looking at.
> 

The following code seems to work fine under clang/gcc, x86_64/aarch64
(also tested in lkdtm_CFI_BACKWARD_SHADOW):

#include <stdio.h>

static __attribute__((noinline))
void set_return_addr(unsigned long *expected, unsigned long *addr)
{
     /* Use of volatile is to make sure final write isn't seen as a dead store. */
     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;

     /* Make sure we've found the right place on the stack before writing it. */
     if(*ret_addr == expected)
         *ret_addr = (addr);
}

static volatile int force_label;

int main(void)
{
     void *array[] = {0, &&normal, &&redirected};

     if (force_label) {
         /* Call it with a NULL to avoid parameters being treated as constants in -02. */
         set_return_addr(NULL, NULL);
         goto * array[force_label];
     }

     do {

         set_return_addr(&&normal, &&redirected);

normal:
         printf("I should be skipped\n");
         break;

redirected:
         printf("Redirected\n");

     } while (0);

     return 0;
}

But currently it still crashes when I try to enable
"-mbranch-protection=pac-ret+leaf+bti".

Because the address of "&&redirected" is not encrypted under pac,
the autiasp check will fail when set_return_addr returns, and
eventually cause the function to crash when it returns to "&&redirected"
("&&redirected" as a reserved label always seems to start with a bti j
insn).

For lkdtm, if we're going to handle both cases in one function, maybe
it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
and maybe also turn off -O2 options for the function :)

Thanks,
Dan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote:
> The following code seems to work fine under clang/gcc, x86_64/aarch64
> (also tested in lkdtm_CFI_BACKWARD_SHADOW):
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>     /* Use of volatile is to make sure final write isn't seen as a dead store. */
>     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>     /* Make sure we've found the right place on the stack before writing it. */
>     if(*ret_addr == expected)
>         *ret_addr = (addr);
> }
> 
> static volatile int force_label;
> 
> int main(void)
> {
>     void *array[] = {0, &&normal, &&redirected};
> 
>     if (force_label) {
>         /* Call it with a NULL to avoid parameters being treated as constants in -02. */
>         set_return_addr(NULL, NULL);
>         goto * array[force_label];
>     }

Hah! I like that. :) I had a weird switch statement that was working for
me; this is cleaner.

> 
>     do {
> 
>         set_return_addr(&&normal, &&redirected);
> 
> normal:
>         printf("I should be skipped\n");
>         break;
> 
> redirected:
>         printf("Redirected\n");
> 
>     } while (0);
> 
>     return 0;
> }
> 
> But currently it still crashes when I try to enable
> "-mbranch-protection=pac-ret+leaf+bti".
> 
> Because the address of "&&redirected" is not encrypted under pac,
> the autiasp check will fail when set_return_addr returns, and
> eventually cause the function to crash when it returns to "&&redirected"
> ("&&redirected" as a reserved label always seems to start with a bti j
> insn).

Strictly speaking, this is entirely correct. :)

> For lkdtm, if we're going to handle both cases in one function, maybe
> it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
> and maybe also turn off -O2 options for the function :)

If we can apply a function attribute to turn off pac for the "does this
work without protections", that should be sufficient.

-- 
Kees Cook

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Kees Cook]

On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote:
> The following code seems to work fine under clang/gcc, x86_64/aarch64
> (also tested in lkdtm_CFI_BACKWARD_SHADOW):
> 
> #include <stdio.h>
> 
> static __attribute__((noinline))
> void set_return_addr(unsigned long *expected, unsigned long *addr)
> {
>     /* Use of volatile is to make sure final write isn't seen as a dead store. */
>     unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> 
>     /* Make sure we've found the right place on the stack before writing it. */
>     if(*ret_addr == expected)
>         *ret_addr = (addr);
> }
> 
> static volatile int force_label;
> 
> int main(void)
> {
>     void *array[] = {0, &&normal, &&redirected};
> 
>     if (force_label) {
>         /* Call it with a NULL to avoid parameters being treated as constants in -02. */
>         set_return_addr(NULL, NULL);
>         goto * array[force_label];
>     }

Hah! I like that. :) I had a weird switch statement that was working for
me; this is cleaner.

> 
>     do {
> 
>         set_return_addr(&&normal, &&redirected);
> 
> normal:
>         printf("I should be skipped\n");
>         break;
> 
> redirected:
>         printf("Redirected\n");
> 
>     } while (0);
> 
>     return 0;
> }
> 
> But currently it still crashes when I try to enable
> "-mbranch-protection=pac-ret+leaf+bti".
> 
> Because the address of "&&redirected" is not encrypted under pac,
> the autiasp check will fail when set_return_addr returns, and
> eventually cause the function to crash when it returns to "&&redirected"
> ("&&redirected" as a reserved label always seems to start with a bti j
> insn).

Strictly speaking, this is entirely correct. :)

> For lkdtm, if we're going to handle both cases in one function, maybe
> it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
> and maybe also turn off -O2 options for the function :)

If we can apply a function attribute to turn off pac for the "does this
work without protections", that should be sufficient.

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/9/22 12:16, Kees Cook wrote:
> On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote:
>> But currently it still crashes when I try to enable
>> "-mbranch-protection=pac-ret+leaf+bti".
>>
>> Because the address of "&&redirected" is not encrypted under pac,
>> the autiasp check will fail when set_return_addr returns, and
>> eventually cause the function to crash when it returns to "&&redirected"
>> ("&&redirected" as a reserved label always seems to start with a bti j
>> insn).
> 
> Strictly speaking, this is entirely correct. :)
> 
>> For lkdtm, if we're going to handle both cases in one function, maybe
>> it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
>> and maybe also turn off -O2 options for the function :)
> 
> If we can apply a function attribute to turn off pac for the "does this
> work without protections", that should be sufficient.
> 

Got it, will do in the next version :)

Thanks,
Dan.

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/9/22 12:16, Kees Cook wrote:
> On Mon, Mar 07, 2022 at 07:16:36AM -0800, Dan Li wrote:
>> But currently it still crashes when I try to enable
>> "-mbranch-protection=pac-ret+leaf+bti".
>>
>> Because the address of "&&redirected" is not encrypted under pac,
>> the autiasp check will fail when set_return_addr returns, and
>> eventually cause the function to crash when it returns to "&&redirected"
>> ("&&redirected" as a reserved label always seems to start with a bti j
>> insn).
> 
> Strictly speaking, this is entirely correct. :)
> 
>> For lkdtm, if we're going to handle both cases in one function, maybe
>> it would be better to turn off the -mbranch-protection=pac-ret+leaf+bti
>> and maybe also turn off -O2 options for the function :)
> 
> If we can apply a function attribute to turn off pac for the "does this
> work without protections", that should be sufficient.
> 

Got it, will do in the next version :)

Thanks,
Dan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 10:42, Kees Cook wrote:
> On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote:
>> Add tests for SCS (Shadow Call Stack) based
>> backward CFI (as implemented by Clang and GCC).
> 
> Cool; thanks for writing these!
> 
>> +lkdtm-$(CONFIG_LKDTM)		+= scs.o
> 
> I'd expect these to be in cfi.c, rather than making a new source file.
> 

Got it.

>> +static noinline void lkdtm_scs_clear_lr(void)
>> +{
>> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
>> +
>> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
> 
> Is the asm needed here? Why not:
> 
> 	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> 
> 	*lr = 0;
> 

Yeah, with "volatile", this one looks better.

>> +
>> +/*
>> + * This tries to call a function protected by Shadow Call Stack,
>> + * which corrupts its own return address during execution.
>> + * Due to the protection, the corruption will not take effect
>> + * when the function returns.
>> + */
>> +void lkdtm_CFI_BACKWARD_SHADOW(void)
> 
> I think these two tests should be collapsed into a single one.
> 

It seems that there is currently no cross-line matching in
selftests/lkdtm/run.sh, if we put these two into one function and
assume we could make noscs_set_lr _survivable_ (like in your example).

Then we could only match "CFI_BACKWARD_SHADOW ok: scs takes effect."
in texts.txt

But if the test result is:
XPASS: Unexpectedly survived lr corruption without scs?
ok: scs takes effect.

It may not be a real pass, but the xxx_set_lr function doesn't work.

>> +{
>> +#ifdef CONFIG_ARM64
>> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
>> +		return;
>> +	}
>> +
>> +	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
>> +	lkdtm_scs_clear_lr();
>> +
>> +	pr_err("ok: scs takes effect.\n");
>> +#else
>> +	pr_err("XFAIL: this test is arm64-only\n");
>> +#endif
> 
> This is slightly surprising -- we have no detection when a function has
> its non-shadow-stack return address corrupted: it just _ignores_ the
> value stored there. That seems like a missed opportunity for warning
> about an unexpected state.
> 

Yes.
Actually I used to try in the plugin to add a detection before the function
returns, and call a callback when a mismatch is found. But since almost
every function has to be instrumented, the performance penalty is
improved from <3% to ~20% (rough calculation, should still be optimized).

>> +}
>> +
>> +/*
>> + * This tries to call a function not protected by Shadow Call Stack,
>> + * which corrupts its own return address during execution.
>> + */
>> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
>> +{
>> +#ifdef CONFIG_ARM64
>> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
>> +		return;
> 
> Other tests try to give some hints about failures, e.g.:
> 
> 		pr_err("FAIL: cannot change for SCS\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 
> Though, having the IS_ENABLED in there makes me wonder if this test
> should instead be made _survivable_ on failure. Something like this,
> completely untested:
> 
> 
> #ifdef CONFIG_ARM64
> static noinline void lkdtm_scs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> 
> /* Function with __noscs attribute clears its return address. */
> static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> #endif
> 
> 
> void lkdtm_CFI_BACKWARD_SHADOW(void)
> {
> #ifdef CONFIG_ARM64
> 
> 	/* Verify the "normal" condition of LR corruption working. */
> 	do {
> 		/* Keep label in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto unexpected;
> 
> 		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> 		lkdtm_noscs_set_lr(&&expected);
> 
> unexpected:
> 		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> 		break;
> 
> expected:
> 		pr_err("ok: lr corruption redirected without scs.\n");
> 	} while (0);
> 
> 
> 	do {
> 		/* Keep labe in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto good_scs;
> 
> 		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> 		lkdtm_scs_set_lr(&&bad_scs);
> 
> good_scs:
> 		pr_info("ok: scs takes effect.\n");
> 		break;
> 
> bad_scs:
> 		pr_err("FAIL: return address rewritten!\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 	} while (0);
> #else
> 	pr_err("XFAIL: this test is arm64-only\n");
> #endif
> }
> 

Thanks for the example, Kees :)
This code (with a little modification) works correctly with clang 12,
but to make sure it's always correct, I think we might need to add the
__attribute__((optnone)) attribute to it, because under -O2 the result
doesn't seem to be "very stable" (as in your example in the next email).

> And we should, actually, be able to make the "set_lr" functions be
> arch-specific, leaving the test itself arch-agnostic....
> 

I'm not sure if my understanding is correct, do it means we should
remove the "#ifdef CONFIG_ARM64" in lkdtm_CFI_BACKWARD_SHADOW?

Then we may not be able to distinguish between failures caused by
platform unsupported (XFAIL) and features not enabled (or not
working properly).

Thanks,
Dan.

Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/3/22 10:42, Kees Cook wrote:
> On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote:
>> Add tests for SCS (Shadow Call Stack) based
>> backward CFI (as implemented by Clang and GCC).
> 
> Cool; thanks for writing these!
> 
>> +lkdtm-$(CONFIG_LKDTM)		+= scs.o
> 
> I'd expect these to be in cfi.c, rather than making a new source file.
> 

Got it.

>> +static noinline void lkdtm_scs_clear_lr(void)
>> +{
>> +	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
>> +
>> +	asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
> 
> Is the asm needed here? Why not:
> 
> 	unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> 
> 	*lr = 0;
> 

Yeah, with "volatile", this one looks better.

>> +
>> +/*
>> + * This tries to call a function protected by Shadow Call Stack,
>> + * which corrupts its own return address during execution.
>> + * Due to the protection, the corruption will not take effect
>> + * when the function returns.
>> + */
>> +void lkdtm_CFI_BACKWARD_SHADOW(void)
> 
> I think these two tests should be collapsed into a single one.
> 

It seems that there is currently no cross-line matching in
selftests/lkdtm/run.sh, if we put these two into one function and
assume we could make noscs_set_lr _survivable_ (like in your example).

Then we could only match "CFI_BACKWARD_SHADOW ok: scs takes effect."
in texts.txt

But if the test result is:
XPASS: Unexpectedly survived lr corruption without scs?
ok: scs takes effect.

It may not be a real pass, but the xxx_set_lr function doesn't work.

>> +{
>> +#ifdef CONFIG_ARM64
>> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
>> +		return;
>> +	}
>> +
>> +	pr_info("Trying to corrupt lr in a function with scs protection ...\n");
>> +	lkdtm_scs_clear_lr();
>> +
>> +	pr_err("ok: scs takes effect.\n");
>> +#else
>> +	pr_err("XFAIL: this test is arm64-only\n");
>> +#endif
> 
> This is slightly surprising -- we have no detection when a function has
> its non-shadow-stack return address corrupted: it just _ignores_ the
> value stored there. That seems like a missed opportunity for warning
> about an unexpected state.
> 

Yes.
Actually I used to try in the plugin to add a detection before the function
returns, and call a callback when a mismatch is found. But since almost
every function has to be instrumented, the performance penalty is
improved from <3% to ~20% (rough calculation, should still be optimized).

>> +}
>> +
>> +/*
>> + * This tries to call a function not protected by Shadow Call Stack,
>> + * which corrupts its own return address during execution.
>> + */
>> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
>> +{
>> +#ifdef CONFIG_ARM64
>> +	if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
>> +		pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
>> +		return;
> 
> Other tests try to give some hints about failures, e.g.:
> 
> 		pr_err("FAIL: cannot change for SCS\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 
> Though, having the IS_ENABLED in there makes me wonder if this test
> should instead be made _survivable_ on failure. Something like this,
> completely untested:
> 
> 
> #ifdef CONFIG_ARM64
> static noinline void lkdtm_scs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> 
> /* Function with __noscs attribute clears its return address. */
> static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
> {
> 	unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
> 	*lr = addr;
> }
> #endif
> 
> 
> void lkdtm_CFI_BACKWARD_SHADOW(void)
> {
> #ifdef CONFIG_ARM64
> 
> 	/* Verify the "normal" condition of LR corruption working. */
> 	do {
> 		/* Keep label in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto unexpected;
> 
> 		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> 		lkdtm_noscs_set_lr(&&expected);
> 
> unexpected:
> 		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> 		break;
> 
> expected:
> 		pr_err("ok: lr corruption redirected without scs.\n");
> 	} while (0);
> 
> 
> 	do {
> 		/* Keep labe in scope to avoid compiler warning. */
> 		if ((volatile int)0)
> 			goto good_scs;
> 
> 		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> 		lkdtm_scs_set_lr(&&bad_scs);
> 
> good_scs:
> 		pr_info("ok: scs takes effect.\n");
> 		break;
> 
> bad_scs:
> 		pr_err("FAIL: return address rewritten!\n");
> 		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> 	} while (0);
> #else
> 	pr_err("XFAIL: this test is arm64-only\n");
> #endif
> }
> 

Thanks for the example, Kees :)
This code (with a little modification) works correctly with clang 12,
but to make sure it's always correct, I think we might need to add the
__attribute__((optnone)) attribute to it, because under -O2 the result
doesn't seem to be "very stable" (as in your example in the next email).

> And we should, actually, be able to make the "set_lr" functions be
> arch-specific, leaving the test itself arch-agnostic....
> 

I'm not sure if my understanding is correct, do it means we should
remove the "#ifdef CONFIG_ARM64" in lkdtm_CFI_BACKWARD_SHADOW?

Then we may not be able to distinguish between failures caused by
platform unsupported (XFAIL) and features not enabled (or not
working properly).

Thanks,
Dan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Add tests for SCS (Shadow Call Stack) based backward CFI.

Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 arch/arm64/include/asm/compiler.h       | 18 ++++++
 drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
 drivers/misc/lkdtm/core.c               |  1 +
 drivers/misc/lkdtm/lkdtm.h              |  1 +
 include/linux/compiler-clang.h          |  1 +
 include/linux/compiler-gcc.h            |  2 +
 include/linux/compiler_types.h          |  4 ++
 tools/testing/selftests/lkdtm/tests.txt |  1 +
 8 files changed, 112 insertions(+)

diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index dc3ea4080e2e..96590fb4a8de 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -8,6 +8,24 @@
 #define ARM64_ASM_PREAMBLE
 #endif
 
+#ifndef __ASSEMBLY__
+#ifdef __KERNEL__
+
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+
+#ifdef CONFIG_ARM64_BTI_KERNEL
+# define __no_ptrauth __attribute__((target("branch-protection=bti")))
+#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
+# define __no_ptrauth __attribute__((target("branch-protection=none")))
+#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
+# define __no_ptrauth __attribute__((target("sign-return-address=none")))
+#endif
+
+#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
+
+#endif /* __KERNEL__ */
+#endif /* __ASSEMBLY__ */
+
 /*
  * The EL0/EL1 pointer bits used by a pointer authentication code.
  * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
index c9aeddef1044..468ba2f26f74 100644
--- a/drivers/misc/lkdtm/cfi.c
+++ b/drivers/misc/lkdtm/cfi.c
@@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
 	pr_err("FAIL: survived mismatched prototype function call!\n");
 	pr_expected_config(CONFIG_CFI_CLANG);
 }
+
+#ifdef CONFIG_ARM64
+/*
+ * This function is used to modify its return address. The PAC needs to be turned
+ * off here to ensure that the modification of the return address will not be blocked.
+ */
+static noinline __no_ptrauth
+void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
+{
+	/* Use of volatile is to make sure final write isn't seen as a dead store. */
+	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
+
+	/* Make sure we've found the right place on the stack before writing it. */
+	if (*ret_addr == expected)
+		*ret_addr = addr;
+}
+
+/* Function with __noscs attribute attempts to modify its return address. */
+static noinline __no_ptrauth __noscs
+void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
+{
+	/* Use of volatile is to make sure final write isn't seen as a dead store. */
+	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
+
+	/* Make sure we've found the right place on the stack before writing it. */
+	if (*ret_addr == expected)
+		*ret_addr = addr;
+}
+#else
+static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
+static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
+#endif
+
+static volatile unsigned int force_label;
+
+/*
+ * This first checks whether a function with the __noscs attribute under
+ * the current platform can directly modify its return address, and if so,
+ * checks whether scs takes effect.
+ */
+void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
+{
+	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
+
+	if (force_label && (force_label < sizeof(array))) {
+		/*
+		 * Call them with "NULL" first to avoid
+		 * arguments being treated as constants in -02.
+		 */
+		lkdtm_noscs_set_lr(NULL, NULL);
+		lkdtm_scs_set_lr(NULL, NULL);
+		goto *array[force_label];
+	}
+
+	/* Keep labels in scope to avoid compiler warnings. */
+	do {
+		/* Verify the "normal" condition of LR corruption working. */
+		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
+		lkdtm_noscs_set_lr(&&unexpected, &&expected);
+
+unexpected:
+		/*
+		 * If lr cannot be modified, the following check is meaningless,
+		 * returns directly.
+		 */
+		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
+		break;
+
+expected:
+		pr_info("ok: lr corruption redirected without scs.\n");
+
+		/* Verify that SCS is in effect. */
+		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
+		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
+
+good_scs:
+		pr_info("ok: scs takes effect.\n");
+		break;
+
+bad_scs:
+		pr_err("FAIL: return address rewritten!\n");
+		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
+	} while (0);
+}
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index f69b964b9952..7af7268b82e4 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(STACKLEAK_ERASING),
 	CRASHTYPE(CFI_FORWARD_PROTO),
+	CRASHTYPE(CFI_BACKWARD_SHADOW),
 	CRASHTYPE(FORTIFIED_OBJECT),
 	CRASHTYPE(FORTIFIED_SUBOBJECT),
 	CRASHTYPE(FORTIFIED_STRSCPY),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index d6137c70ebbe..a66fba949ab5 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
 
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
+void lkdtm_CFI_BACKWARD_SHADOW(void);
 
 /* fortify.c */
 void lkdtm_FORTIFIED_OBJECT(void);
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 3c4de9b6c6e3..2db37db36651 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -68,3 +68,4 @@
 
 #define __nocfi		__attribute__((__no_sanitize__("cfi")))
 #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
+#define __no_optimize	__attribute__((optnone))
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index deff5b308470..28d1b0ec6656 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -162,3 +162,5 @@
 #if GCC_VERSION < 90100
 #undef __alloc_size__
 #endif
+
+#define __no_optimize	__attribute__((optimize("-O0")))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3c1795fdb568..f5ad83f7ea2f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -257,6 +257,10 @@ struct ftrace_likely_data {
 # define __nocfi
 #endif
 
+#ifndef __no_ptrauth
+# define __no_ptrauth
+#endif
+
 #ifndef __cficanonical
 # define __cficanonical
 #endif
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..12df67a3b419 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
 USERCOPY_KERNEL
 STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
 CFI_FORWARD_PROTO
+CFI_BACKWARD_SHADOW ok: scs takes effect
 FORTIFIED_STRSCPY
 FORTIFIED_OBJECT
 FORTIFIED_SUBOBJECT
-- 
2.17.1

[PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Add tests for SCS (Shadow Call Stack) based backward CFI.

Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
---
 arch/arm64/include/asm/compiler.h       | 18 ++++++
 drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
 drivers/misc/lkdtm/core.c               |  1 +
 drivers/misc/lkdtm/lkdtm.h              |  1 +
 include/linux/compiler-clang.h          |  1 +
 include/linux/compiler-gcc.h            |  2 +
 include/linux/compiler_types.h          |  4 ++
 tools/testing/selftests/lkdtm/tests.txt |  1 +
 8 files changed, 112 insertions(+)

diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index dc3ea4080e2e..96590fb4a8de 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -8,6 +8,24 @@
 #define ARM64_ASM_PREAMBLE
 #endif
 
+#ifndef __ASSEMBLY__
+#ifdef __KERNEL__
+
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+
+#ifdef CONFIG_ARM64_BTI_KERNEL
+# define __no_ptrauth __attribute__((target("branch-protection=bti")))
+#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
+# define __no_ptrauth __attribute__((target("branch-protection=none")))
+#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
+# define __no_ptrauth __attribute__((target("sign-return-address=none")))
+#endif
+
+#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
+
+#endif /* __KERNEL__ */
+#endif /* __ASSEMBLY__ */
+
 /*
  * The EL0/EL1 pointer bits used by a pointer authentication code.
  * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
index c9aeddef1044..468ba2f26f74 100644
--- a/drivers/misc/lkdtm/cfi.c
+++ b/drivers/misc/lkdtm/cfi.c
@@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
 	pr_err("FAIL: survived mismatched prototype function call!\n");
 	pr_expected_config(CONFIG_CFI_CLANG);
 }
+
+#ifdef CONFIG_ARM64
+/*
+ * This function is used to modify its return address. The PAC needs to be turned
+ * off here to ensure that the modification of the return address will not be blocked.
+ */
+static noinline __no_ptrauth
+void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
+{
+	/* Use of volatile is to make sure final write isn't seen as a dead store. */
+	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
+
+	/* Make sure we've found the right place on the stack before writing it. */
+	if (*ret_addr == expected)
+		*ret_addr = addr;
+}
+
+/* Function with __noscs attribute attempts to modify its return address. */
+static noinline __no_ptrauth __noscs
+void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
+{
+	/* Use of volatile is to make sure final write isn't seen as a dead store. */
+	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
+
+	/* Make sure we've found the right place on the stack before writing it. */
+	if (*ret_addr == expected)
+		*ret_addr = addr;
+}
+#else
+static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
+static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
+#endif
+
+static volatile unsigned int force_label;
+
+/*
+ * This first checks whether a function with the __noscs attribute under
+ * the current platform can directly modify its return address, and if so,
+ * checks whether scs takes effect.
+ */
+void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
+{
+	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
+
+	if (force_label && (force_label < sizeof(array))) {
+		/*
+		 * Call them with "NULL" first to avoid
+		 * arguments being treated as constants in -02.
+		 */
+		lkdtm_noscs_set_lr(NULL, NULL);
+		lkdtm_scs_set_lr(NULL, NULL);
+		goto *array[force_label];
+	}
+
+	/* Keep labels in scope to avoid compiler warnings. */
+	do {
+		/* Verify the "normal" condition of LR corruption working. */
+		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
+		lkdtm_noscs_set_lr(&&unexpected, &&expected);
+
+unexpected:
+		/*
+		 * If lr cannot be modified, the following check is meaningless,
+		 * returns directly.
+		 */
+		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
+		break;
+
+expected:
+		pr_info("ok: lr corruption redirected without scs.\n");
+
+		/* Verify that SCS is in effect. */
+		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
+		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
+
+good_scs:
+		pr_info("ok: scs takes effect.\n");
+		break;
+
+bad_scs:
+		pr_err("FAIL: return address rewritten!\n");
+		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
+	} while (0);
+}
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index f69b964b9952..7af7268b82e4 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
 	CRASHTYPE(USERCOPY_KERNEL),
 	CRASHTYPE(STACKLEAK_ERASING),
 	CRASHTYPE(CFI_FORWARD_PROTO),
+	CRASHTYPE(CFI_BACKWARD_SHADOW),
 	CRASHTYPE(FORTIFIED_OBJECT),
 	CRASHTYPE(FORTIFIED_SUBOBJECT),
 	CRASHTYPE(FORTIFIED_STRSCPY),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index d6137c70ebbe..a66fba949ab5 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
 
 /* cfi.c */
 void lkdtm_CFI_FORWARD_PROTO(void);
+void lkdtm_CFI_BACKWARD_SHADOW(void);
 
 /* fortify.c */
 void lkdtm_FORTIFIED_OBJECT(void);
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 3c4de9b6c6e3..2db37db36651 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -68,3 +68,4 @@
 
 #define __nocfi		__attribute__((__no_sanitize__("cfi")))
 #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
+#define __no_optimize	__attribute__((optnone))
diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
index deff5b308470..28d1b0ec6656 100644
--- a/include/linux/compiler-gcc.h
+++ b/include/linux/compiler-gcc.h
@@ -162,3 +162,5 @@
 #if GCC_VERSION < 90100
 #undef __alloc_size__
 #endif
+
+#define __no_optimize	__attribute__((optimize("-O0")))
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 3c1795fdb568..f5ad83f7ea2f 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -257,6 +257,10 @@ struct ftrace_likely_data {
 # define __nocfi
 #endif
 
+#ifndef __no_ptrauth
+# define __no_ptrauth
+#endif
+
 #ifndef __cficanonical
 # define __cficanonical
 #endif
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 6b36b7f5dcf9..12df67a3b419 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
 USERCOPY_KERNEL
 STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
 CFI_FORWARD_PROTO
+CFI_BACKWARD_SHADOW ok: scs takes effect
 FORTIFIED_STRSCPY
 FORTIFIED_OBJECT
 FORTIFIED_SUBOBJECT
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
Hi, Kees,

With the default -O2, this code tests fine in gcc 11/clang 12, but
doesn't work in gcc 7.5.0. In 7.5.0, the generated code is as follows:

  bl      ffff8000088335c0 <lkdtm_noscs_set_lr>
  nop     						      ## return address of lkdtm_noscs_set_lr
  adrp    x0, ffff80000962b000 <kallsyms_token_index+0xe5908>  ## address of "&&unexpected"

The address of "&&unexpected" is still not guaranteed to always be the
same as the return address of lkdtm_noscs_set_lr, so I had to add
__no_optimize attribute here.

The code compiled under __no_optimize in above versions works fine, but
I saw the following description in the gcc user manual:

"You may not use this mechanism to jump to code in a different function.
If you do that, totally unpredictable things happen."

So there might be some risk here that we might not be able to run this
test case stably across all compiler versions, probably we still have to
use two test cases to complete this.

link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Labels-as-Values.html#Labels-as-Values

Thanks,
Dan.

> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
Hi, Kees,

With the default -O2, this code tests fine in gcc 11/clang 12, but
doesn't work in gcc 7.5.0. In 7.5.0, the generated code is as follows:

  bl      ffff8000088335c0 <lkdtm_noscs_set_lr>
  nop     						      ## return address of lkdtm_noscs_set_lr
  adrp    x0, ffff80000962b000 <kallsyms_token_index+0xe5908>  ## address of "&&unexpected"

The address of "&&unexpected" is still not guaranteed to always be the
same as the return address of lkdtm_noscs_set_lr, so I had to add
__no_optimize attribute here.

The code compiled under __no_optimize in above versions works fine, but
I saw the following description in the gcc user manual:

"You may not use this mechanism to jump to code in a different function.
If you do that, totally unpredictable things happen."

So there might be some risk here that we might not be able to run this
test case stably across all compiler versions, probably we still have to
use two test cases to complete this.

link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Labels-as-Values.html#Labels-as-Values

Thanks,
Dan.

> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Hi Kees,

Gentile ping for this :).

I also saw the discussion on llvm-project, use address of labels as a
parameter doesn't seem to be stable.

Do we need to split it into two cases here?

Link: https://github.com/llvm/llvm-project/issues/54328

Thanks,
Dan


On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>   arch/arm64/include/asm/compiler.h       | 18 ++++++
>   drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
>   drivers/misc/lkdtm/core.c               |  1 +
>   drivers/misc/lkdtm/lkdtm.h              |  1 +
>   include/linux/compiler-clang.h          |  1 +
>   include/linux/compiler-gcc.h            |  2 +
>   include/linux/compiler_types.h          |  4 ++
>   tools/testing/selftests/lkdtm/tests.txt |  1 +
>   8 files changed, 112 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..96590fb4a8de 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -8,6 +8,24 @@
>   #define ARM64_ASM_PREAMBLE
>   #endif
>   
> +#ifndef __ASSEMBLY__
> +#ifdef __KERNEL__
> +
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +
> +#ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_ptrauth __attribute__((target("branch-protection=bti")))
> +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
> +# define __no_ptrauth __attribute__((target("branch-protection=none")))
> +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
> +# define __no_ptrauth __attribute__((target("sign-return-address=none")))
> +#endif
> +
> +#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASSEMBLY__ */
> +
>   /*
>    * The EL0/EL1 pointer bits used by a pointer authentication code.
>    * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index c9aeddef1044..468ba2f26f74 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
>   	pr_err("FAIL: survived mismatched prototype function call!\n");
>   	pr_expected_config(CONFIG_CFI_CLANG);
>   }
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..7af7268b82e4 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
>   	CRASHTYPE(USERCOPY_KERNEL),
>   	CRASHTYPE(STACKLEAK_ERASING),
>   	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
>   	CRASHTYPE(FORTIFIED_OBJECT),
>   	CRASHTYPE(FORTIFIED_SUBOBJECT),
>   	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a66fba949ab5 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>   
>   /* cfi.c */
>   void lkdtm_CFI_FORWARD_PROTO(void);
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
>   
>   /* fortify.c */
>   void lkdtm_FORTIFIED_OBJECT(void);
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3c4de9b6c6e3..2db37db36651 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -68,3 +68,4 @@
>   
>   #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>   #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
> +#define __no_optimize	__attribute__((optnone))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index deff5b308470..28d1b0ec6656 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -162,3 +162,5 @@
>   #if GCC_VERSION < 90100
>   #undef __alloc_size__
>   #endif
> +
> +#define __no_optimize	__attribute__((optimize("-O0")))
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3c1795fdb568..f5ad83f7ea2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -257,6 +257,10 @@ struct ftrace_likely_data {
>   # define __nocfi
>   #endif
>   
> +#ifndef __no_ptrauth
> +# define __no_ptrauth
> +#endif
> +
>   #ifndef __cficanonical
>   # define __cficanonical
>   #endif
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 6b36b7f5dcf9..12df67a3b419 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
>   USERCOPY_KERNEL
>   STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>   CFI_FORWARD_PROTO
> +CFI_BACKWARD_SHADOW ok: scs takes effect
>   FORTIFIED_STRSCPY
>   FORTIFIED_OBJECT
>   FORTIFIED_SUBOBJECT

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Hi Kees,

Gentile ping for this :).

I also saw the discussion on llvm-project, use address of labels as a
parameter doesn't seem to be stable.

Do we need to split it into two cases here?

Link: https://github.com/llvm/llvm-project/issues/54328

Thanks,
Dan


On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>   arch/arm64/include/asm/compiler.h       | 18 ++++++
>   drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
>   drivers/misc/lkdtm/core.c               |  1 +
>   drivers/misc/lkdtm/lkdtm.h              |  1 +
>   include/linux/compiler-clang.h          |  1 +
>   include/linux/compiler-gcc.h            |  2 +
>   include/linux/compiler_types.h          |  4 ++
>   tools/testing/selftests/lkdtm/tests.txt |  1 +
>   8 files changed, 112 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..96590fb4a8de 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -8,6 +8,24 @@
>   #define ARM64_ASM_PREAMBLE
>   #endif
>   
> +#ifndef __ASSEMBLY__
> +#ifdef __KERNEL__
> +
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +
> +#ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_ptrauth __attribute__((target("branch-protection=bti")))
> +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
> +# define __no_ptrauth __attribute__((target("branch-protection=none")))
> +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
> +# define __no_ptrauth __attribute__((target("sign-return-address=none")))
> +#endif
> +
> +#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASSEMBLY__ */
> +
>   /*
>    * The EL0/EL1 pointer bits used by a pointer authentication code.
>    * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index c9aeddef1044..468ba2f26f74 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
>   	pr_err("FAIL: survived mismatched prototype function call!\n");
>   	pr_expected_config(CONFIG_CFI_CLANG);
>   }
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..7af7268b82e4 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
>   	CRASHTYPE(USERCOPY_KERNEL),
>   	CRASHTYPE(STACKLEAK_ERASING),
>   	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
>   	CRASHTYPE(FORTIFIED_OBJECT),
>   	CRASHTYPE(FORTIFIED_SUBOBJECT),
>   	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a66fba949ab5 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>   
>   /* cfi.c */
>   void lkdtm_CFI_FORWARD_PROTO(void);
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
>   
>   /* fortify.c */
>   void lkdtm_FORTIFIED_OBJECT(void);
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3c4de9b6c6e3..2db37db36651 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -68,3 +68,4 @@
>   
>   #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>   #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
> +#define __no_optimize	__attribute__((optnone))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index deff5b308470..28d1b0ec6656 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -162,3 +162,5 @@
>   #if GCC_VERSION < 90100
>   #undef __alloc_size__
>   #endif
> +
> +#define __no_optimize	__attribute__((optimize("-O0")))
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3c1795fdb568..f5ad83f7ea2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -257,6 +257,10 @@ struct ftrace_likely_data {
>   # define __nocfi
>   #endif
>   
> +#ifndef __no_ptrauth
> +# define __no_ptrauth
> +#endif
> +
>   #ifndef __cficanonical
>   # define __cficanonical
>   #endif
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 6b36b7f5dcf9..12df67a3b419 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
>   USERCOPY_KERNEL
>   STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>   CFI_FORWARD_PROTO
> +CFI_BACKWARD_SHADOW ok: scs takes effect
>   FORTIFIED_STRSCPY
>   FORTIFIED_OBJECT
>   FORTIFIED_SUBOBJECT

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Hi Kees,

Gentile ping for this :).

I also saw the discussion on llvm-project, use address of labels as a
parameter doesn't seem to be stable.

Do we need to split it into two cases here?

Link: https://github.com/llvm/llvm-project/issues/54328

Thanks,
Dan


On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>   arch/arm64/include/asm/compiler.h       | 18 ++++++
>   drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
>   drivers/misc/lkdtm/core.c               |  1 +
>   drivers/misc/lkdtm/lkdtm.h              |  1 +
>   include/linux/compiler-clang.h          |  1 +
>   include/linux/compiler-gcc.h            |  2 +
>   include/linux/compiler_types.h          |  4 ++
>   tools/testing/selftests/lkdtm/tests.txt |  1 +
>   8 files changed, 112 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..96590fb4a8de 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -8,6 +8,24 @@
>   #define ARM64_ASM_PREAMBLE
>   #endif
>   
> +#ifndef __ASSEMBLY__
> +#ifdef __KERNEL__
> +
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +
> +#ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_ptrauth __attribute__((target("branch-protection=bti")))
> +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
> +# define __no_ptrauth __attribute__((target("branch-protection=none")))
> +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
> +# define __no_ptrauth __attribute__((target("sign-return-address=none")))
> +#endif
> +
> +#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASSEMBLY__ */
> +
>   /*
>    * The EL0/EL1 pointer bits used by a pointer authentication code.
>    * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index c9aeddef1044..468ba2f26f74 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
>   	pr_err("FAIL: survived mismatched prototype function call!\n");
>   	pr_expected_config(CONFIG_CFI_CLANG);
>   }
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..7af7268b82e4 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
>   	CRASHTYPE(USERCOPY_KERNEL),
>   	CRASHTYPE(STACKLEAK_ERASING),
>   	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
>   	CRASHTYPE(FORTIFIED_OBJECT),
>   	CRASHTYPE(FORTIFIED_SUBOBJECT),
>   	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a66fba949ab5 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>   
>   /* cfi.c */
>   void lkdtm_CFI_FORWARD_PROTO(void);
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
>   
>   /* fortify.c */
>   void lkdtm_FORTIFIED_OBJECT(void);
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3c4de9b6c6e3..2db37db36651 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -68,3 +68,4 @@
>   
>   #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>   #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
> +#define __no_optimize	__attribute__((optnone))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index deff5b308470..28d1b0ec6656 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -162,3 +162,5 @@
>   #if GCC_VERSION < 90100
>   #undef __alloc_size__
>   #endif
> +
> +#define __no_optimize	__attribute__((optimize("-O0")))
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3c1795fdb568..f5ad83f7ea2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -257,6 +257,10 @@ struct ftrace_likely_data {
>   # define __nocfi
>   #endif
>   
> +#ifndef __no_ptrauth
> +# define __no_ptrauth
> +#endif
> +
>   #ifndef __cficanonical
>   # define __cficanonical
>   #endif
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 6b36b7f5dcf9..12df67a3b419 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
>   USERCOPY_KERNEL
>   STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>   CFI_FORWARD_PROTO
> +CFI_BACKWARD_SHADOW ok: scs takes effect
>   FORTIFIED_STRSCPY
>   FORTIFIED_OBJECT
>   FORTIFIED_SUBOBJECT

Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

[Dan Li]

Hi Kees,

Gentile ping for this :).

I also saw the discussion on llvm-project, use address of labels as a
parameter doesn't seem to be stable.

Do we need to split it into two cases here?

Link: https://github.com/llvm/llvm-project/issues/54328

Thanks,
Dan


On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
> ---
>   arch/arm64/include/asm/compiler.h       | 18 ++++++
>   drivers/misc/lkdtm/cfi.c                | 84 +++++++++++++++++++++++++
>   drivers/misc/lkdtm/core.c               |  1 +
>   drivers/misc/lkdtm/lkdtm.h              |  1 +
>   include/linux/compiler-clang.h          |  1 +
>   include/linux/compiler-gcc.h            |  2 +
>   include/linux/compiler_types.h          |  4 ++
>   tools/testing/selftests/lkdtm/tests.txt |  1 +
>   8 files changed, 112 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
> index dc3ea4080e2e..96590fb4a8de 100644
> --- a/arch/arm64/include/asm/compiler.h
> +++ b/arch/arm64/include/asm/compiler.h
> @@ -8,6 +8,24 @@
>   #define ARM64_ASM_PREAMBLE
>   #endif
>   
> +#ifndef __ASSEMBLY__
> +#ifdef __KERNEL__
> +
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +
> +#ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_ptrauth __attribute__((target("branch-protection=bti")))
> +#elif defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
> +# define __no_ptrauth __attribute__((target("branch-protection=none")))
> +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
> +# define __no_ptrauth __attribute__((target("sign-return-address=none")))
> +#endif
> +
> +#endif /* CONFIG_ARM64_PTR_AUTH_KERNEL */
> +
> +#endif /* __KERNEL__ */
> +#endif /* __ASSEMBLY__ */
> +
>   /*
>    * The EL0/EL1 pointer bits used by a pointer authentication code.
>    * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index c9aeddef1044..468ba2f26f74 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -41,3 +41,87 @@ void lkdtm_CFI_FORWARD_PROTO(void)
>   	pr_err("FAIL: survived mismatched prototype function call!\n");
>   	pr_expected_config(CONFIG_CFI_CLANG);
>   }
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr)
> +{
> +	/* Use of volatile is to make sure final write isn't seen as a dead store. */
> +	unsigned long * volatile *ret_addr = (unsigned long **)__builtin_frame_address(0) + 1;
> +
> +	/* Make sure we've found the right place on the stack before writing it. */
> +	if (*ret_addr == expected)
> +		*ret_addr = addr;
> +}
> +#else
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&bad_scs);
> +
> +good_scs:
> +		pr_info("ok: scs takes effect.\n");
> +		break;
> +
> +bad_scs:
> +		pr_err("FAIL: return address rewritten!\n");
> +		pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> +	} while (0);
> +}
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..7af7268b82e4 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,7 @@ static const struct crashtype crashtypes[] = {
>   	CRASHTYPE(USERCOPY_KERNEL),
>   	CRASHTYPE(STACKLEAK_ERASING),
>   	CRASHTYPE(CFI_FORWARD_PROTO),
> +	CRASHTYPE(CFI_BACKWARD_SHADOW),
>   	CRASHTYPE(FORTIFIED_OBJECT),
>   	CRASHTYPE(FORTIFIED_SUBOBJECT),
>   	CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a66fba949ab5 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -157,6 +157,7 @@ void lkdtm_STACKLEAK_ERASING(void);
>   
>   /* cfi.c */
>   void lkdtm_CFI_FORWARD_PROTO(void);
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
>   
>   /* fortify.c */
>   void lkdtm_FORTIFIED_OBJECT(void);
> diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
> index 3c4de9b6c6e3..2db37db36651 100644
> --- a/include/linux/compiler-clang.h
> +++ b/include/linux/compiler-clang.h
> @@ -68,3 +68,4 @@
>   
>   #define __nocfi		__attribute__((__no_sanitize__("cfi")))
>   #define __cficanonical	__attribute__((__cfi_canonical_jump_table__))
> +#define __no_optimize	__attribute__((optnone))
> diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> index deff5b308470..28d1b0ec6656 100644
> --- a/include/linux/compiler-gcc.h
> +++ b/include/linux/compiler-gcc.h
> @@ -162,3 +162,5 @@
>   #if GCC_VERSION < 90100
>   #undef __alloc_size__
>   #endif
> +
> +#define __no_optimize	__attribute__((optimize("-O0")))
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 3c1795fdb568..f5ad83f7ea2f 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -257,6 +257,10 @@ struct ftrace_likely_data {
>   # define __nocfi
>   #endif
>   
> +#ifndef __no_ptrauth
> +# define __no_ptrauth
> +#endif
> +
>   #ifndef __cficanonical
>   # define __cficanonical
>   #endif
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 6b36b7f5dcf9..12df67a3b419 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -73,6 +73,7 @@ USERCOPY_STACK_BEYOND
>   USERCOPY_KERNEL
>   STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
>   CFI_FORWARD_PROTO
> +CFI_BACKWARD_SHADOW ok: scs takes effect
>   FORTIFIED_STRSCPY
>   FORTIFIED_OBJECT
>   FORTIFIED_SUBOBJECT

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel