[v2] lkdtm: Add CFI_BACKWARD to test ROP mitigations

Commit Message

Kees Cook April 16, 2022, 12:11 a.m. UTC

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 <[e-mail]>
Signed-off-by: Dan Li <[e-mail]>
Cc: Arnd Bergmann <[e-mail]>
Cc: Greg Kroah-Hartman <[e-mail]>
Signed-off-by: Kees Cook <[e-mail]>
---
v1: https://lore.kernel.org/lkml/[e-mail]
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(+)

Comments

Dan Li April 17, 2022, 9:15 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Dan Li <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>
> ---
> v1: https://lore.kernel.org/lkml/[e-mail]
> 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 :)

Kees Cook April 18, 2022, 9:51 p.m. UTC | #2

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 <[e-mail]>
> > Signed-off-by: Dan Li <[e-mail]>
> > Cc: Arnd Bergmann <[e-mail]>
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Signed-off-by: Kees Cook <[e-mail]>
> > ---
> > v1: https://lore.kernel.org/lkml/[e-mail]
> > 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. :)

Daniel Díaz Dec. 7, 2022, 12:28 a.m. UTC | #3

Hello!

On Sat, 16 Apr 2022 at 00:30, Kees Cook <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Dan Li <[e-mail]>
> Cc: Arnd Bergmann <[e-mail]>
> Cc: Greg Kroah-Hartman <[e-mail]>
> Signed-off-by: Kees Cook <[e-mail]>
> ---
> v1: https://lore.kernel.org/lkml/[e-mail]
> 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
[e-mail]

Kees Cook Dec. 8, 2022, 6:22 a.m. UTC | #4

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 <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Dan Li <[e-mail]>
> > Cc: Arnd Bergmann <[e-mail]>
> > Cc: Greg Kroah-Hartman <[e-mail]>
> > Signed-off-by: Kees Cook <[e-mail]>
> > ---
> > v1: https://lore.kernel.org/lkml/[e-mail]
> > 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?

Kristina Martšenko Dec. 9, 2022, 5:34 p.m. UTC | #5

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 <[e-mail]> 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 <[e-mail]>
>>> Signed-off-by: Dan Li <[e-mail]>
>>> Cc: Arnd Bergmann <[e-mail]>
>>> Cc: Greg Kroah-Hartman <[e-mail]>
>>> Signed-off-by: Kees Cook <[e-mail]>
>>> ---
>>> v1: https://lore.kernel.org/lkml/[e-mail]
>>> 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 <[e-mail]>
Signed-off-by: Kristina Martsenko <[e-mail]>
Link: https://lore.kernel.org/all/[e-mail]/
---
 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 Dec. 14, 2022, 10:48 p.m. UTC | #6

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 <[e-mail]>
> Signed-off-by: Kristina Martsenko <[e-mail]>
> Link: https://lore.kernel.org/all/[e-mail]/

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
>

Patch

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