2022-04-16 02:45:43

by Kees Cook

[permalink] [raw]
Subject: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

In order to test various backward-edge control flow integrity methods,
add a test that manipulates the return address on the stack. Currently
only arm64 Pointer Authentication and Shadow Call Stack is supported.

$ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT

Under SCS, successful test of the mitigation is reported as:

lkdtm: Performing direct entry CFI_BACKWARD
lkdtm: Attempting unchecked stack return address redirection ...
lkdtm: ok: redirected stack return address.
lkdtm: Attempting checked stack return address redirection ...
lkdtm: ok: control flow unchanged.

Under PAC, successful test of the mitigation is reported by the PAC
exception handler:

lkdtm: Performing direct entry CFI_BACKWARD
lkdtm: Attempting unchecked stack return address redirection ...
lkdtm: ok: redirected stack return address.
lkdtm: Attempting checked stack return address redirection ...
Unable to handle kernel paging request at virtual address bfffffc0088d0514
Mem abort info:
ESR = 0x86000004
EC = 0x21: IABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 0x04: level 0 translation fault
[bfffffc0088d0514] address between user and kernel address ranges
...

If the CONFIGs are missing (or the mitigation isn't working), failure
is reported as:

lkdtm: Performing direct entry CFI_BACKWARD
lkdtm: Attempting unchecked stack return address redirection ...
lkdtm: ok: redirected stack return address.
lkdtm: Attempting checked stack return address redirection ...
lkdtm: FAIL: stack return address was redirected!
lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y

Co-developed-by: Dan Li <[email protected]>
Signed-off-by: Dan Li <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Kees Cook <[email protected]>
---
v1: https://lore.kernel.org/lkml/[email protected]
v2:
- add PAGE_OFFSET setting for PAC bits (Dan Li)
---
drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
tools/testing/selftests/lkdtm/tests.txt | 1 +
2 files changed, 135 insertions(+)

diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
index e88f778be0d5..804965a480b7 100644
--- a/drivers/misc/lkdtm/cfi.c
+++ b/drivers/misc/lkdtm/cfi.c
@@ -3,6 +3,7 @@
* This is for all the tests relating directly to Control Flow Integrity.
*/
#include "lkdtm.h"
+#include <asm/page.h>

static int called_count;

@@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
pr_expected_config(CONFIG_CFI_CLANG);
}

+/*
+ * This can stay local to LKDTM, as there should not be a production reason
+ * to disable PAC && SCS.
+ */
+#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
+# ifdef CONFIG_ARM64_BTI_KERNEL
+# define __no_pac "branch-protection=bti"
+# else
+# define __no_pac "branch-protection=none"
+# endif
+# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
+#else
+# define __no_ret_protection __noscs
+#endif
+
+#define no_pac_addr(addr) \
+ ((__force __typeof__(addr))((__force u64)(addr) | PAGE_OFFSET))
+
+/* The ultimate ROP gadget. */
+static noinline __no_ret_protection
+void set_return_addr_unchecked(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 (no_pac_addr(*ret_addr) == expected)
+ *ret_addr = (addr);
+ else
+ /* Check architecture, stack layout, or compiler behavior... */
+ pr_warn("Eek: return address mismatch! %px != %px\n",
+ *ret_addr, addr);
+}
+
+static 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 (no_pac_addr(*ret_addr) == expected)
+ *ret_addr = (addr);
+ else
+ /* Check architecture, stack layout, or compiler behavior... */
+ pr_warn("Eek: return address mismatch! %px != %px\n",
+ *ret_addr, addr);
+}
+
+static volatile int force_check;
+
+static void lkdtm_CFI_BACKWARD(void)
+{
+ /* Use calculated gotos to keep labels addressable. */
+ void *labels[] = {0, &&normal, &&redirected, &&check_normal, &&check_redirected};
+
+ pr_info("Attempting unchecked stack return address redirection ...\n");
+
+ /* Always false */
+ if (force_check) {
+ /*
+ * Prepare to call with NULLs to avoid parameters being treated as
+ * constants in -02.
+ */
+ set_return_addr_unchecked(NULL, NULL);
+ set_return_addr(NULL, NULL);
+ if (force_check)
+ goto *labels[1];
+ if (force_check)
+ goto *labels[2];
+ if (force_check)
+ goto *labels[3];
+ if (force_check)
+ goto *labels[4];
+ return;
+ }
+
+ /*
+ * Use fallthrough switch case to keep basic block ordering between
+ * set_return_addr*() and the label after it.
+ */
+ switch (force_check) {
+ case 0:
+ set_return_addr_unchecked(&&normal, &&redirected);
+ fallthrough;
+ case 1:
+normal:
+ /* Always true */
+ if (!force_check) {
+ pr_err("FAIL: stack return address manipulation failed!\n");
+ /* If we can't redirect "normally", we can't test mitigations. */
+ return;
+ }
+ break;
+ default:
+redirected:
+ pr_info("ok: redirected stack return address.\n");
+ break;
+ }
+
+ pr_info("Attempting checked stack return address redirection ...\n");
+
+ switch (force_check) {
+ case 0:
+ set_return_addr(&&check_normal, &&check_redirected);
+ fallthrough;
+ case 1:
+check_normal:
+ /* Always true */
+ if (!force_check) {
+ pr_info("ok: control flow unchanged.\n");
+ return;
+ }
+
+check_redirected:
+ pr_err("FAIL: stack return address was redirected!\n");
+ break;
+ }
+
+ if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) {
+ pr_expected_config(CONFIG_ARM64_PTR_AUTH_KERNEL);
+ return;
+ }
+ if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
+ pr_expected_config(CONFIG_SHADOW_CALL_STACK);
+ return;
+ }
+ pr_warn("This is probably expected, since this %s was built *without* %s=y nor %s=y\n",
+ lkdtm_kernel_info,
+ "CONFIG_ARM64_PTR_AUTH_KERNEL", "CONFIG_SHADOW_CALL_STACK");
+}
+
static struct crashtype crashtypes[] = {
CRASHTYPE(CFI_FORWARD_PROTO),
+ CRASHTYPE(CFI_BACKWARD),
};

struct crashtype_category cfi_crashtypes = {
diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
index 243c781f0780..9dace01dbf15 100644
--- a/tools/testing/selftests/lkdtm/tests.txt
+++ b/tools/testing/selftests/lkdtm/tests.txt
@@ -74,6 +74,7 @@ USERCOPY_STACK_BEYOND
USERCOPY_KERNEL
STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
CFI_FORWARD_PROTO
+CFI_BACKWARD call trace:|ok: control flow unchanged
FORTIFIED_STRSCPY
FORTIFIED_OBJECT
FORTIFIED_SUBOBJECT
--
2.32.0


2022-04-19 05:07:49

by Dan Li

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations



On 4/15/22 17:11, Kees Cook wrote:
> In order to test various backward-edge control flow integrity methods,
> add a test that manipulates the return address on the stack. Currently
> only arm64 Pointer Authentication and Shadow Call Stack is supported.
>
> $ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT
>
> Under SCS, successful test of the mitigation is reported as:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> lkdtm: ok: control flow unchanged.
>
> Under PAC, successful test of the mitigation is reported by the PAC
> exception handler:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> Unable to handle kernel paging request at virtual address bfffffc0088d0514
> Mem abort info:
> ESR = 0x86000004
> EC = 0x21: IABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x04: level 0 translation fault
> [bfffffc0088d0514] address between user and kernel address ranges
> ...
>
> If the CONFIGs are missing (or the mitigation isn't working), failure
> is reported as:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> lkdtm: FAIL: stack return address was redirected!
> lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y
>
> Co-developed-by: Dan Li <[email protected]>
> Signed-off-by: Dan Li <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> v1: https://lore.kernel.org/lkml/[email protected]
> v2:
> - add PAGE_OFFSET setting for PAC bits (Dan Li)
> ---
> drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
> tools/testing/selftests/lkdtm/tests.txt | 1 +
> 2 files changed, 135 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index e88f778be0d5..804965a480b7 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -3,6 +3,7 @@
> * This is for all the tests relating directly to Control Flow Integrity.
> */
> #include "lkdtm.h"
> +#include <asm/page.h>
>
> static int called_count;
>
> @@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
> pr_expected_config(CONFIG_CFI_CLANG);
> }
>
> +/*
> + * This can stay local to LKDTM, as there should not be a production reason
> + * to disable PAC && SCS.
> + */
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +# ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_pac "branch-protection=bti"
> +# else
> +# define __no_pac "branch-protection=none"
> +# endif
> +# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
> +#else
> +# define __no_ret_protection __noscs
> +#endif
> +
> +#define no_pac_addr(addr) \
> + ((__force __typeof__(addr))((__force u64)(addr) | PAGE_OFFSET))
> +
> +/* The ultimate ROP gadget. */
> +static noinline __no_ret_protection
> +void set_return_addr_unchecked(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 (no_pac_addr(*ret_addr) == expected)
> + *ret_addr = (addr);
> + else
> + /* Check architecture, stack layout, or compiler behavior... */
> + pr_warn("Eek: return address mismatch! %px != %px\n",
> + *ret_addr, addr);
> +}
> +
> +static 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 (no_pac_addr(*ret_addr) == expected)
> + *ret_addr = (addr);
> + else
> + /* Check architecture, stack layout, or compiler behavior... */
> + pr_warn("Eek: return address mismatch! %px != %px\n",
> + *ret_addr, addr);
> +}
> +
> +static volatile int force_check;
> +
> +static void lkdtm_CFI_BACKWARD(void)
> +{
> + /* Use calculated gotos to keep labels addressable. */
> + void *labels[] = {0, &&normal, &&redirected, &&check_normal, &&check_redirected};
> +
> + pr_info("Attempting unchecked stack return address redirection ...\n");
> +
> + /* Always false */
> + if (force_check) {
> + /*
> + * Prepare to call with NULLs to avoid parameters being treated as
> + * constants in -02.
> + */
> + set_return_addr_unchecked(NULL, NULL);
> + set_return_addr(NULL, NULL);
> + if (force_check)
> + goto *labels[1];
> + if (force_check)
> + goto *labels[2];
> + if (force_check)
> + goto *labels[3];
> + if (force_check)
> + goto *labels[4];
> + return;
> + }
> +
> + /*
> + * Use fallthrough switch case to keep basic block ordering between
> + * set_return_addr*() and the label after it.
> + */
> + switch (force_check) {
> + case 0:
> + set_return_addr_unchecked(&&normal, &&redirected);
> + fallthrough;
> + case 1:
> +normal:
> + /* Always true */
> + if (!force_check) {
> + pr_err("FAIL: stack return address manipulation failed!\n");
> + /* If we can't redirect "normally", we can't test mitigations. */
> + return;
> + }
> + break;
> + default:
> +redirected:
> + pr_info("ok: redirected stack return address.\n");
> + break;
> + }
> +
> + pr_info("Attempting checked stack return address redirection ...\n");
> +
> + switch (force_check) {
> + case 0:
> + set_return_addr(&&check_normal, &&check_redirected);
> + fallthrough;
> + case 1:
> +check_normal:
> + /* Always true */
> + if (!force_check) {
> + pr_info("ok: control flow unchanged.\n");
> + return;
> + }
> +
> +check_redirected:
> + pr_err("FAIL: stack return address was redirected!\n");
> + break;
> + }
> +
> + if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) {
> + pr_expected_config(CONFIG_ARM64_PTR_AUTH_KERNEL);
> + return;
> + }
> + if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> + pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> + return;
> + }
> + pr_warn("This is probably expected, since this %s was built *without* %s=y nor %s=y\n",
> + lkdtm_kernel_info,
> + "CONFIG_ARM64_PTR_AUTH_KERNEL", "CONFIG_SHADOW_CALL_STACK");
> +}
> +
> static struct crashtype crashtypes[] = {
> CRASHTYPE(CFI_FORWARD_PROTO),
> + CRASHTYPE(CFI_BACKWARD),
> };
>
> struct crashtype_category cfi_crashtypes = {
> diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> index 243c781f0780..9dace01dbf15 100644
> --- a/tools/testing/selftests/lkdtm/tests.txt
> +++ b/tools/testing/selftests/lkdtm/tests.txt
> @@ -74,6 +74,7 @@ USERCOPY_STACK_BEYOND
> USERCOPY_KERNEL
> STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
> CFI_FORWARD_PROTO
> +CFI_BACKWARD call trace:|ok: control flow unchanged
> FORTIFIED_STRSCPY
> FORTIFIED_OBJECT
> FORTIFIED_SUBOBJECT


Compiling with gcc/llvm 12 on aarch64 platform with scs/pac enabled
respectively, all four cases work fine for me :)

2022-04-20 09:11:16

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

On Sun, Apr 17, 2022 at 02:15:43AM -0700, Dan Li wrote:
>
>
> On 4/15/22 17:11, Kees Cook wrote:
> > In order to test various backward-edge control flow integrity methods,
> > add a test that manipulates the return address on the stack. Currently
> > only arm64 Pointer Authentication and Shadow Call Stack is supported.
> >
> > $ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT
> >
> > Under SCS, successful test of the mitigation is reported as:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > lkdtm: ok: control flow unchanged.
> >
> > Under PAC, successful test of the mitigation is reported by the PAC
> > exception handler:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > Unable to handle kernel paging request at virtual address bfffffc0088d0514
> > Mem abort info:
> > ESR = 0x86000004
> > EC = 0x21: IABT (current EL), IL = 32 bits
> > SET = 0, FnV = 0
> > EA = 0, S1PTW = 0
> > FSC = 0x04: level 0 translation fault
> > [bfffffc0088d0514] address between user and kernel address ranges
> > ...
> >
> > If the CONFIGs are missing (or the mitigation isn't working), failure
> > is reported as:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > lkdtm: FAIL: stack return address was redirected!
> > lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y
> >
> > Co-developed-by: Dan Li <[email protected]>
> > Signed-off-by: Dan Li <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > v1: https://lore.kernel.org/lkml/[email protected]
> > v2:
> > - add PAGE_OFFSET setting for PAC bits (Dan Li)
> > ---
> > drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
> > tools/testing/selftests/lkdtm/tests.txt | 1 +
> > 2 files changed, 135 insertions(+)
> >
> > diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> > index e88f778be0d5..804965a480b7 100644
> > --- a/drivers/misc/lkdtm/cfi.c
> > +++ b/drivers/misc/lkdtm/cfi.c
> > @@ -3,6 +3,7 @@
> > * This is for all the tests relating directly to Control Flow Integrity.
> > */
> > #include "lkdtm.h"
> > +#include <asm/page.h>
> > static int called_count;
> > @@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
> > pr_expected_config(CONFIG_CFI_CLANG);
> > }
> > +/*
> > + * This can stay local to LKDTM, as there should not be a production reason
> > + * to disable PAC && SCS.
> > + */
> > +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> > +# ifdef CONFIG_ARM64_BTI_KERNEL
> > +# define __no_pac "branch-protection=bti"
> > +# else
> > +# define __no_pac "branch-protection=none"
> > +# endif
> > +# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
> > +#else
> > +# define __no_ret_protection __noscs
> > +#endif
> > +
> > +#define no_pac_addr(addr) \
> > + ((__force __typeof__(addr))((__force u64)(addr) | PAGE_OFFSET))
> > +
> > +/* The ultimate ROP gadget. */
> > +static noinline __no_ret_protection
> > +void set_return_addr_unchecked(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 (no_pac_addr(*ret_addr) == expected)
> > + *ret_addr = (addr);
> > + else
> > + /* Check architecture, stack layout, or compiler behavior... */
> > + pr_warn("Eek: return address mismatch! %px != %px\n",
> > + *ret_addr, addr);
> > +}
> > +
> > +static 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 (no_pac_addr(*ret_addr) == expected)
> > + *ret_addr = (addr);
> > + else
> > + /* Check architecture, stack layout, or compiler behavior... */
> > + pr_warn("Eek: return address mismatch! %px != %px\n",
> > + *ret_addr, addr);
> > +}
> > +
> > +static volatile int force_check;
> > +
> > +static void lkdtm_CFI_BACKWARD(void)
> > +{
> > + /* Use calculated gotos to keep labels addressable. */
> > + void *labels[] = {0, &&normal, &&redirected, &&check_normal, &&check_redirected};
> > +
> > + pr_info("Attempting unchecked stack return address redirection ...\n");
> > +
> > + /* Always false */
> > + if (force_check) {
> > + /*
> > + * Prepare to call with NULLs to avoid parameters being treated as
> > + * constants in -02.
> > + */
> > + set_return_addr_unchecked(NULL, NULL);
> > + set_return_addr(NULL, NULL);
> > + if (force_check)
> > + goto *labels[1];
> > + if (force_check)
> > + goto *labels[2];
> > + if (force_check)
> > + goto *labels[3];
> > + if (force_check)
> > + goto *labels[4];
> > + return;
> > + }
> > +
> > + /*
> > + * Use fallthrough switch case to keep basic block ordering between
> > + * set_return_addr*() and the label after it.
> > + */
> > + switch (force_check) {
> > + case 0:
> > + set_return_addr_unchecked(&&normal, &&redirected);
> > + fallthrough;
> > + case 1:
> > +normal:
> > + /* Always true */
> > + if (!force_check) {
> > + pr_err("FAIL: stack return address manipulation failed!\n");
> > + /* If we can't redirect "normally", we can't test mitigations. */
> > + return;
> > + }
> > + break;
> > + default:
> > +redirected:
> > + pr_info("ok: redirected stack return address.\n");
> > + break;
> > + }
> > +
> > + pr_info("Attempting checked stack return address redirection ...\n");
> > +
> > + switch (force_check) {
> > + case 0:
> > + set_return_addr(&&check_normal, &&check_redirected);
> > + fallthrough;
> > + case 1:
> > +check_normal:
> > + /* Always true */
> > + if (!force_check) {
> > + pr_info("ok: control flow unchanged.\n");
> > + return;
> > + }
> > +
> > +check_redirected:
> > + pr_err("FAIL: stack return address was redirected!\n");
> > + break;
> > + }
> > +
> > + if (IS_ENABLED(CONFIG_ARM64_PTR_AUTH_KERNEL)) {
> > + pr_expected_config(CONFIG_ARM64_PTR_AUTH_KERNEL);
> > + return;
> > + }
> > + if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> > + pr_expected_config(CONFIG_SHADOW_CALL_STACK);
> > + return;
> > + }
> > + pr_warn("This is probably expected, since this %s was built *without* %s=y nor %s=y\n",
> > + lkdtm_kernel_info,
> > + "CONFIG_ARM64_PTR_AUTH_KERNEL", "CONFIG_SHADOW_CALL_STACK");
> > +}
> > +
> > static struct crashtype crashtypes[] = {
> > CRASHTYPE(CFI_FORWARD_PROTO),
> > + CRASHTYPE(CFI_BACKWARD),
> > };
> > struct crashtype_category cfi_crashtypes = {
> > diff --git a/tools/testing/selftests/lkdtm/tests.txt b/tools/testing/selftests/lkdtm/tests.txt
> > index 243c781f0780..9dace01dbf15 100644
> > --- a/tools/testing/selftests/lkdtm/tests.txt
> > +++ b/tools/testing/selftests/lkdtm/tests.txt
> > @@ -74,6 +74,7 @@ USERCOPY_STACK_BEYOND
> > USERCOPY_KERNEL
> > STACKLEAK_ERASING OK: the rest of the thread stack is properly erased
> > CFI_FORWARD_PROTO
> > +CFI_BACKWARD call trace:|ok: control flow unchanged
> > FORTIFIED_STRSCPY
> > FORTIFIED_OBJECT
> > FORTIFIED_SUBOBJECT
>
>
> Compiling with gcc/llvm 12 on aarch64 platform with scs/pac enabled
> respectively, all four cases work fine for me :)

Great! Thanks for confirming it. :)

--
Kees Cook

2022-12-07 01:45:04

by Daniel Díaz

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

Hello!

On Sat, 16 Apr 2022 at 00:30, Kees Cook <[email protected]> wrote:
> In order to test various backward-edge control flow integrity methods,
> add a test that manipulates the return address on the stack. Currently
> only arm64 Pointer Authentication and Shadow Call Stack is supported.
>
> $ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT
>
> Under SCS, successful test of the mitigation is reported as:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> lkdtm: ok: control flow unchanged.
>
> Under PAC, successful test of the mitigation is reported by the PAC
> exception handler:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> Unable to handle kernel paging request at virtual address bfffffc0088d0514
> Mem abort info:
> ESR = 0x86000004
> EC = 0x21: IABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> FSC = 0x04: level 0 translation fault
> [bfffffc0088d0514] address between user and kernel address ranges
> ...
>
> If the CONFIGs are missing (or the mitigation isn't working), failure
> is reported as:
>
> lkdtm: Performing direct entry CFI_BACKWARD
> lkdtm: Attempting unchecked stack return address redirection ...
> lkdtm: ok: redirected stack return address.
> lkdtm: Attempting checked stack return address redirection ...
> lkdtm: FAIL: stack return address was redirected!
> lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y
>
> Co-developed-by: Dan Li <[email protected]>
> Signed-off-by: Dan Li <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> v1: https://lore.kernel.org/lkml/[email protected]
> v2:
> - add PAGE_OFFSET setting for PAC bits (Dan Li)
> ---
> drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
> tools/testing/selftests/lkdtm/tests.txt | 1 +
> 2 files changed, 135 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index e88f778be0d5..804965a480b7 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -3,6 +3,7 @@
> * This is for all the tests relating directly to Control Flow Integrity.
> */
> #include "lkdtm.h"
> +#include <asm/page.h>
>
> static int called_count;
>
> @@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
> pr_expected_config(CONFIG_CFI_CLANG);
> }
>
> +/*
> + * This can stay local to LKDTM, as there should not be a production reason
> + * to disable PAC && SCS.
> + */
> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> +# ifdef CONFIG_ARM64_BTI_KERNEL
> +# define __no_pac "branch-protection=bti"
> +# else
> +# define __no_pac "branch-protection=none"
> +# endif
> +# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
> +#else
> +# define __no_ret_protection __noscs
> +#endif

We're seeing this problem with allmodconfig on arm64 and GCC 8 (this
one observed on 6.0.12-rc3):

-----8<----------8<----------8<-----
make --silent --keep-going --jobs=8
O=/home/tuxbuild/.cache/tuxmake/builds/2/build
CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- ARCH=arm64
CROSS_COMPILE=aarch64-linux-gnu- 'CC=sccache aarch64-linux-gnu-gcc'
'HOSTCC=sccache gcc'
/builds/linux/drivers/misc/lkdtm/cfi.c:67:1: error: pragma or
attribute 'target("branch-protection=none")' is not valid
{
^
make[4]: *** [/builds/linux/scripts/Makefile.build:249:
drivers/misc/lkdtm/cfi.o] Error 1
make[4]: Target '__build' not remade because of errors.
make[3]: *** [/builds/linux/scripts/Makefile.build:465:
drivers/misc/lkdtm] Error 2
make[3]: Target '__build' not remade because of errors.
make[2]: *** [/builds/linux/scripts/Makefile.build:465: drivers/misc] Error 2
make[2]: Target '__build' not remade because of errors.
make[1]: *** [/builds/linux/Makefile:1852: drivers] Error 2
----->8---------->8---------->8-----

Reproducer: `tuxmake --runtime podman --target-arch arm64 --toolchain
gcc-8 --kconfig allmodconfig
CROSS_COMPILE_COMPAT=arm-linux-gnueabihf-`

Is this a legit problem?

Greetings!

Daniel Díaz
[email protected]

2022-12-08 07:08:20

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

On Tue, Dec 06, 2022 at 06:28:53PM -0600, Daniel D?az wrote:
> Hello!
>
> On Sat, 16 Apr 2022 at 00:30, Kees Cook <[email protected]> wrote:
> > In order to test various backward-edge control flow integrity methods,
> > add a test that manipulates the return address on the stack. Currently
> > only arm64 Pointer Authentication and Shadow Call Stack is supported.
> >
> > $ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT
> >
> > Under SCS, successful test of the mitigation is reported as:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > lkdtm: ok: control flow unchanged.
> >
> > Under PAC, successful test of the mitigation is reported by the PAC
> > exception handler:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > Unable to handle kernel paging request at virtual address bfffffc0088d0514
> > Mem abort info:
> > ESR = 0x86000004
> > EC = 0x21: IABT (current EL), IL = 32 bits
> > SET = 0, FnV = 0
> > EA = 0, S1PTW = 0
> > FSC = 0x04: level 0 translation fault
> > [bfffffc0088d0514] address between user and kernel address ranges
> > ...
> >
> > If the CONFIGs are missing (or the mitigation isn't working), failure
> > is reported as:
> >
> > lkdtm: Performing direct entry CFI_BACKWARD
> > lkdtm: Attempting unchecked stack return address redirection ...
> > lkdtm: ok: redirected stack return address.
> > lkdtm: Attempting checked stack return address redirection ...
> > lkdtm: FAIL: stack return address was redirected!
> > lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y
> >
> > Co-developed-by: Dan Li <[email protected]>
> > Signed-off-by: Dan Li <[email protected]>
> > Cc: Arnd Bergmann <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Kees Cook <[email protected]>
> > ---
> > v1: https://lore.kernel.org/lkml/[email protected]
> > v2:
> > - add PAGE_OFFSET setting for PAC bits (Dan Li)
> > ---
> > drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
> > tools/testing/selftests/lkdtm/tests.txt | 1 +
> > 2 files changed, 135 insertions(+)
> >
> > diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> > index e88f778be0d5..804965a480b7 100644
> > --- a/drivers/misc/lkdtm/cfi.c
> > +++ b/drivers/misc/lkdtm/cfi.c
> > @@ -3,6 +3,7 @@
> > * This is for all the tests relating directly to Control Flow Integrity.
> > */
> > #include "lkdtm.h"
> > +#include <asm/page.h>
> >
> > static int called_count;
> >
> > @@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
> > pr_expected_config(CONFIG_CFI_CLANG);
> > }
> >
> > +/*
> > + * This can stay local to LKDTM, as there should not be a production reason
> > + * to disable PAC && SCS.
> > + */
> > +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
> > +# ifdef CONFIG_ARM64_BTI_KERNEL
> > +# define __no_pac "branch-protection=bti"
> > +# else
> > +# define __no_pac "branch-protection=none"
> > +# endif
> > +# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
> > +#else
> > +# define __no_ret_protection __noscs
> > +#endif
>
> We're seeing this problem with allmodconfig on arm64 and GCC 8 (this
> one observed on 6.0.12-rc3):
>
> -----8<----------8<----------8<-----
> make --silent --keep-going --jobs=8
> O=/home/tuxbuild/.cache/tuxmake/builds/2/build
> CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- ARCH=arm64
> CROSS_COMPILE=aarch64-linux-gnu- 'CC=sccache aarch64-linux-gnu-gcc'
> 'HOSTCC=sccache gcc'
> /builds/linux/drivers/misc/lkdtm/cfi.c:67:1: error: pragma or
> attribute 'target("branch-protection=none")' is not valid
> {
> ^

Uuuh... how is CONFIG_ARM64_PTR_AUTH_KERNEL getting set if the compiler
can't support the 'target("branch-protection=none")' attribute?

--
Kees Cook

2022-12-09 18:20:28

by Kristina Martsenko

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

On 08/12/2022 06:22, Kees Cook wrote:
> On Tue, Dec 06, 2022 at 06:28:53PM -0600, Daniel D�az wrote:
>> Hello!
>>
>> On Sat, 16 Apr 2022 at 00:30, Kees Cook <[email protected]> wrote:
>>> In order to test various backward-edge control flow integrity methods,
>>> add a test that manipulates the return address on the stack. Currently
>>> only arm64 Pointer Authentication and Shadow Call Stack is supported.
>>>
>>> $ echo CFI_BACKWARD | cat >/sys/kernel/debug/provoke-crash/DIRECT
>>>
>>> Under SCS, successful test of the mitigation is reported as:
>>>
>>> lkdtm: Performing direct entry CFI_BACKWARD
>>> lkdtm: Attempting unchecked stack return address redirection ...
>>> lkdtm: ok: redirected stack return address.
>>> lkdtm: Attempting checked stack return address redirection ...
>>> lkdtm: ok: control flow unchanged.
>>>
>>> Under PAC, successful test of the mitigation is reported by the PAC
>>> exception handler:
>>>
>>> lkdtm: Performing direct entry CFI_BACKWARD
>>> lkdtm: Attempting unchecked stack return address redirection ...
>>> lkdtm: ok: redirected stack return address.
>>> lkdtm: Attempting checked stack return address redirection ...
>>> Unable to handle kernel paging request at virtual address bfffffc0088d0514
>>> Mem abort info:
>>> ESR = 0x86000004
>>> EC = 0x21: IABT (current EL), IL = 32 bits
>>> SET = 0, FnV = 0
>>> EA = 0, S1PTW = 0
>>> FSC = 0x04: level 0 translation fault
>>> [bfffffc0088d0514] address between user and kernel address ranges
>>> ...
>>>
>>> If the CONFIGs are missing (or the mitigation isn't working), failure
>>> is reported as:
>>>
>>> lkdtm: Performing direct entry CFI_BACKWARD
>>> lkdtm: Attempting unchecked stack return address redirection ...
>>> lkdtm: ok: redirected stack return address.
>>> lkdtm: Attempting checked stack return address redirection ...
>>> lkdtm: FAIL: stack return address was redirected!
>>> lkdtm: This is probably expected, since this kernel was built *without* CONFIG_ARM64_PTR_AUTH_KERNEL=y nor CONFIG_SHADOW_CALL_STACK=y
>>>
>>> Co-developed-by: Dan Li <[email protected]>
>>> Signed-off-by: Dan Li <[email protected]>
>>> Cc: Arnd Bergmann <[email protected]>
>>> Cc: Greg Kroah-Hartman <[email protected]>
>>> Signed-off-by: Kees Cook <[email protected]>
>>> ---
>>> v1: https://lore.kernel.org/lkml/[email protected]
>>> v2:
>>> - add PAGE_OFFSET setting for PAC bits (Dan Li)
>>> ---
>>> drivers/misc/lkdtm/cfi.c | 134 ++++++++++++++++++++++++
>>> tools/testing/selftests/lkdtm/tests.txt | 1 +
>>> 2 files changed, 135 insertions(+)
>>>
>>> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
>>> index e88f778be0d5..804965a480b7 100644
>>> --- a/drivers/misc/lkdtm/cfi.c
>>> +++ b/drivers/misc/lkdtm/cfi.c
>>> @@ -3,6 +3,7 @@
>>> * This is for all the tests relating directly to Control Flow Integrity.
>>> */
>>> #include "lkdtm.h"
>>> +#include <asm/page.h>
>>>
>>> static int called_count;
>>>
>>> @@ -42,8 +43,141 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
>>> pr_expected_config(CONFIG_CFI_CLANG);
>>> }
>>>
>>> +/*
>>> + * This can stay local to LKDTM, as there should not be a production reason
>>> + * to disable PAC && SCS.
>>> + */
>>> +#ifdef CONFIG_ARM64_PTR_AUTH_KERNEL
>>> +# ifdef CONFIG_ARM64_BTI_KERNEL
>>> +# define __no_pac "branch-protection=bti"
>>> +# else
>>> +# define __no_pac "branch-protection=none"
>>> +# endif
>>> +# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
>>> +#else
>>> +# define __no_ret_protection __noscs
>>> +#endif
>>
>> We're seeing this problem with allmodconfig on arm64 and GCC 8 (this
>> one observed on 6.0.12-rc3):
>>
>> -----8<----------8<----------8<-----
>> make --silent --keep-going --jobs=8
>> O=/home/tuxbuild/.cache/tuxmake/builds/2/build
>> CROSS_COMPILE_COMPAT=arm-linux-gnueabihf- ARCH=arm64
>> CROSS_COMPILE=aarch64-linux-gnu- 'CC=sccache aarch64-linux-gnu-gcc'
>> 'HOSTCC=sccache gcc'
>> /builds/linux/drivers/misc/lkdtm/cfi.c:67:1: error: pragma or
>> attribute 'target("branch-protection=none")' is not valid
>> {
>> ^
>
> Uuuh... how is CONFIG_ARM64_PTR_AUTH_KERNEL getting set if the compiler
> can't support the 'target("branch-protection=none")' attribute?
>

Older GCC versions supported the (now deprecated) -msign-return-address option
instead of the newer -mbranch-protection option, and the kernel checks for that
too when setting CONFIG_ARM64_PTR_AUTH_KERNEL. I guess the test has never
compiled with older GCC versions. The following patch should fix it.

-- 8< --

Subject: [PATCH] lkdtm: cfi: Make PAC test work with GCC 7 and 8

The CFI test uses the branch-protection=none compiler attribute to
disable PAC return address protection on a function. While newer GCC
versions support this attribute, older versions (GCC 7 and 8) instead
supported the sign-return-address=none attribute, leading to a build
failure when the test is built with older compilers. Fix it by checking
which attribute is supported and using the correct one.

Fixes: 2e53b877dc12 ("lkdtm: Add CFI_BACKWARD to test ROP mitigations")
Reported-by: Daniel Díaz <[email protected]>
Signed-off-by: Kristina Martsenko <[email protected]>
Link: https://lore.kernel.org/all/CAEUSe78kDPxQmQqCWW-_9LCgJDFhAeMoVBFnX9QLx18Z4uT4VQ@mail.gmail.com/
---
drivers/misc/lkdtm/cfi.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
index 5245cf6013c9..d4bb8e31a2fe 100644
--- a/drivers/misc/lkdtm/cfi.c
+++ b/drivers/misc/lkdtm/cfi.c
@@ -54,7 +54,11 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
# ifdef CONFIG_ARM64_BTI_KERNEL
# define __no_pac "branch-protection=bti"
# else
-# define __no_pac "branch-protection=none"
+# ifdef CONFIG_CC_HAS_BRANCH_PROT_PAC_RET
+# define __no_pac "branch-protection=none"
+# else
+# define __no_pac "sign-return-address=none"
+# endif
# endif
# define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
#else

2022-12-14 23:21:18

by Kees Cook

[permalink] [raw]
Subject: Re: [PATCH v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

On Fri, Dec 09, 2022 at 05:34:41PM +0000, Kristina Martsenko wrote:
> Subject: [PATCH] lkdtm: cfi: Make PAC test work with GCC 7 and 8
>
> The CFI test uses the branch-protection=none compiler attribute to
> disable PAC return address protection on a function. While newer GCC
> versions support this attribute, older versions (GCC 7 and 8) instead
> supported the sign-return-address=none attribute, leading to a build
> failure when the test is built with older compilers. Fix it by checking
> which attribute is supported and using the correct one.
>
> Fixes: 2e53b877dc12 ("lkdtm: Add CFI_BACKWARD to test ROP mitigations")
> Reported-by: Daniel D?az <[email protected]>
> Signed-off-by: Kristina Martsenko <[email protected]>
> Link: https://lore.kernel.org/all/CAEUSe78kDPxQmQqCWW-_9LCgJDFhAeMoVBFnX9QLx18Z4uT4VQ@mail.gmail.com/

Thanks! Added to my tree.

> ---
> drivers/misc/lkdtm/cfi.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
> index 5245cf6013c9..d4bb8e31a2fe 100644
> --- a/drivers/misc/lkdtm/cfi.c
> +++ b/drivers/misc/lkdtm/cfi.c
> @@ -54,7 +54,11 @@ static void lkdtm_CFI_FORWARD_PROTO(void)
> # ifdef CONFIG_ARM64_BTI_KERNEL
> # define __no_pac "branch-protection=bti"
> # else
> -# define __no_pac "branch-protection=none"
> +# ifdef CONFIG_CC_HAS_BRANCH_PROT_PAC_RET
> +# define __no_pac "branch-protection=none"
> +# else
> +# define __no_pac "sign-return-address=none"
> +# endif
> # endif
> # define __no_ret_protection __noscs __attribute__((__target__(__no_pac)))
> #else
>

--
Kees Cook