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

[Kristina Martsenko <kristina.martsenko@arm.com>]

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 <keescook@chromium.org> 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 <ashimida@linux.alibaba.com>
>>> Signed-off-by: Dan Li <ashimida@linux.alibaba.com>
>>> Cc: Arnd Bergmann <arnd@arndb.de>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> ---
>>> v1: https://lore.kernel.org/lkml/20220413213917.711770-1-keescook@chromium.org
>>> 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 <daniel.diaz@linaro.org>
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
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