diff mbox series

lkdtm: Add CFI_BACKWARD to test ROP mitigations

Message ID 20220413213917.711770-1-keescook@chromium.org (mailing list archive)
State Superseded
Headers show
Series lkdtm: Add CFI_BACKWARD to test ROP mitigations | expand

Commit Message

Kees Cook April 13, 2022, 9:39 p.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.

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>
---
 drivers/misc/lkdtm/cfi.c                | 130 ++++++++++++++++++++++++
 tools/testing/selftests/lkdtm/tests.txt |   1 +
 2 files changed, 131 insertions(+)

Comments

Dan Li April 14, 2022, 10:19 a.m. UTC | #1 
Hi, Kees,
Thanks for the rewrite. I tested this patch, and it works fine for
me except for a few minor comments below :)

On 4/13/22 14:39, Kees Cook wrote:
> +/* 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(*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(*ret_addr == expected)
> +		*ret_addr = (addr);

When PAC is enabled, I get a mismatch as follows:

/kselftest_install/lkdtm # ./CFI_BACKWARD.sh
[  182.120133] lkdtm: Performing direct entry CFI_BACKWARD
[  182.120665] lkdtm: Attempting unchecked stack return address redirection ...
[  182.122543] lkdtm: ok: redirected stack return address.
[  182.123521] lkdtm: Attempting checked stack return address redirection ...
[  182.123964] lkdtm: Eek: return address mismatch! bfff800008fa8014 != ffff800008fa8030
[  182.124502] lkdtm: ok: control flow unchanged.
CFI_BACKWARD: saw 'call trace:|ok: control flow unchanged': ok

We may need to ignore the pac high bits of return address according
to TCR.T1SZ (or simply remove the high 16 bits before comparing).

> +	else
> +		/* Check architecture, stack layout, or compiler behavior... */
> +		pr_warn("Eek: return address mismatch! %px != %px\n",
> +			*ret_addr, addr);

According to the context, it might be "expected" here?

		pr_warn("Eek: return address mismatch! %px != %px\n",
			*ret_addr, expected);

I simply ignored the upper 16 bits, and tested it separately
in gcc/llvm 12 with SCS/PAC and all the four cases worked fine for me.

Thanks,
Dan.
Kees Cook April 14, 2022, 5:22 p.m. UTC | #2 
On Thu, Apr 14, 2022 at 03:19:02AM -0700, Dan Li wrote:
> Hi, Kees,
> Thanks for the rewrite. I tested this patch, and it works fine for
> me except for a few minor comments below :)
> 
> On 4/13/22 14:39, Kees Cook wrote:
> > +/* 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(*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(*ret_addr == expected)
> > +		*ret_addr = (addr);
> 
> When PAC is enabled, I get a mismatch as follows:
> 
> /kselftest_install/lkdtm # ./CFI_BACKWARD.sh
> [  182.120133] lkdtm: Performing direct entry CFI_BACKWARD
> [  182.120665] lkdtm: Attempting unchecked stack return address redirection ...
> [  182.122543] lkdtm: ok: redirected stack return address.
> [  182.123521] lkdtm: Attempting checked stack return address redirection ...
> [  182.123964] lkdtm: Eek: return address mismatch! bfff800008fa8014 != ffff800008fa8030
> [  182.124502] lkdtm: ok: control flow unchanged.
> CFI_BACKWARD: saw 'call trace:|ok: control flow unchanged': ok
> 
> We may need to ignore the pac high bits of return address according
> to TCR.T1SZ (or simply remove the high 16 bits before comparing).

Oh! Hah, yes, I totally forgot that. Thanks for testing -- getting PAC
emulation working in QEMU has eluded me. I think untagged_addr() will
work yes? i.e.:

	if((untagged_addr(*ret_addr) == expected)

> 
> > +	else
> > +		/* Check architecture, stack layout, or compiler behavior... */
> > +		pr_warn("Eek: return address mismatch! %px != %px\n",
> > +			*ret_addr, addr);
> 
> According to the context, it might be "expected" here?
> 
> 		pr_warn("Eek: return address mismatch! %px != %px\n",
> 			*ret_addr, expected);
> 
> I simply ignored the upper 16 bits, and tested it separately
> in gcc/llvm 12 with SCS/PAC and all the four cases worked fine for me.

Great! Do you have the PAC "Oops" text handy so I can include it in the
commit log as an example of what should be expected?

Thanks!
Dan Li April 15, 2022, 8:39 a.m. UTC | #3 
On 4/14/22 10:22, Kees Cook wrote:
> On Thu, Apr 14, 2022 at 03:19:02AM -0700, Dan Li wrote:
>> Hi, Kees,
>> Thanks for the rewrite. I tested this patch, and it works fine for
>> me except for a few minor comments below :)
>> We may need to ignore the pac high bits of return address according
>> to TCR.T1SZ (or simply remove the high 16 bits before comparing).
> 
> Oh! Hah, yes, I totally forgot that. Thanks for testing -- getting PAC
> emulation working in QEMU has eluded me. I think untagged_addr() will
> work yes? i.e.:
> 
> 	if((untagged_addr(*ret_addr) == expected)
> 

untagged_addr might not clear enough bits, the following code works
fine for me:

+#define no_pac_addr(addr)      \
+       ((__force __typeof__(addr))((__force u64)(addr) | PAGE_OFFSET))
        
-       if(*ret_addr == expected)
+       if(no_pac_addr(*ret_addr) == expected)
                 *ret_addr = (addr);

I re-checked the arm manual and found that the pac bits in an address is:
- Xn[54:bottom_PAC_bit]		 //When address tagging is used
- Xn[63:56, 54:bottom_PAC_bit]	 //When address tagging is not used

bottom_PAC_bit = 64-TCR_ELx.TnSZ //For kernel is VA_BITS

The pac bits could be at most [63:56, 54:VA_BITS], untagged_addr
clears [63:56] (and clearing the high 16 bits doesn't seem to be
enough either :) ).

For example, when CONFIG_ARM64_VA_BITS_39 enabled, i get a case:
- lr         : 0xffffffc0088d04f8
- lr with pac: 0x5681a740088d04f8
- PAGE_OFFSET: 0xffffff8000000000

"lr with pac|PAGE_OFFSET" seems to meet the need.

>>
>>> +	else
>>> +		/* Check architecture, stack layout, or compiler behavior... */
>>> +		pr_warn("Eek: return address mismatch! %px != %px\n",
>>> +			*ret_addr, addr);
>>
>> According to the context, it might be "expected" here?
>>
>> 		pr_warn("Eek: return address mismatch! %px != %px\n",
>> 			*ret_addr, expected);
>>
>> I simply ignored the upper 16 bits, and tested it separately
>> in gcc/llvm 12 with SCS/PAC and all the four cases worked fine for me.
> 
> Great! Do you have the PAC "Oops" text handy so I can include it in the
> commit log as an example of what should be expected?
> 

Yeah, in my test environment I get the following output:

/kselftest_install/lkdtm # ./CFI_BACKWARD.sh
[   58.333529] lkdtm: Performing direct entry CFI_BACKWARD
[   58.333927] lkdtm: Attempting unchecked stack return address redirection ...
[   58.334230] lkdtm: ok: redirected stack return address.
[   58.334870] lkdtm: Attempting checked stack return address redirection ...
[   58.336287] Unable to handle kernel paging request at virtual address bfffffc0088d0514
[   58.336633] Mem abort info:
[   58.336789]   ESR = 0x86000004
[   58.336992]   EC = 0x21: IABT (current EL), IL = 32 bits
[   58.337234]   SET = 0, FnV = 0
[   58.337429]   EA = 0, S1PTW = 0
[   58.337611]   FSC = 0x04: level 0 translation fault
[   58.337874] [bfffffc0088d0514] address between user and kernel address ranges
[   58.338304] Internal error: Oops: 86000004 [#1] PREEMPT SMP
[   58.339209] Modules linked in:
[   58.340384] CPU: 1 PID: 131 Comm: cat Not tainted 5.18.0-rc2-29474-gb8dcca8f6a13-dirty #393
[   58.340842] Hardware name: linux,dummy-virt (DT)
[   58.341231] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   58.341593] pc : 0xbfffffc0088d0514
[   58.342255] lr : lkdtm_CFI_BACKWARD+0x108/0x130
[   58.342716] sp : ffffffc00a73bc60
[   58.342906] x29: ffffffc00a73bc60 x28: ffffff8003320000 x27: 0000000000000002
[   58.343462] x26: ffffffc00a204d00 x25: 0000000000000002 x24: ffffffc0092e72f0
[   58.343863] x23: 0000000000000006 x22: ffffffc00a204d10 x21: ffffff80188d2000
[   58.344264] x20: ffffffc00a73bde0 x19: ffffffc00a302000 x18: ffffffffffffffff
[   58.344732] x17: 663a72646461202c x16: 3866343064383830 x15: 0000000000000004
[   58.345112] x14: 0000000000000fff x13: ffffffc009f8a3e0 x12: 0000000000000003
[   58.345492] x11: 00000000ffffefff x10: c0000000ffffefff x9 : 90af2887c07d9500
[   58.345926] x8 : ffffffc0088d04f8 x7 : 205d323234353333 x6 : 332e38352020205b
[   58.346288] x5 : ffffffc00a2c313c x4 : 0000000000000001 x3 : 0000000000000000
[   58.346670] x2 : 0000000000000000 x1 : ffffffc00968626d x0 : 000000000000006d
[   58.347154] Call trace:
[   58.347419]  0xbfffffc0088d0514
[   58.347806]  lkdtm_do_action+0x1c/0x30
[   58.348085]  direct_entry+0x178/0x1b0
[   58.348291]  full_proxy_write+0x6c/0xe8
[   58.348523]  vfs_write+0x174/0x3b0
[   58.348721]  ksys_write+0x78/0xe4
[   58.348914]  __arm64_sys_write+0x1c/0x28
[   58.349125]  el0_svc_common+0xa4/0x134
[   58.349354]  do_el0_svc+0x24/0x7c
[   58.349551]  el0_svc+0x28/0xa4
[   58.349745]  el0t_64_sync_handler+0x84/0xe4
[   58.349995]  el0t_64_sync+0x170/0x174
[   58.350659] Code: bad PC value
[   58.351182] ---[ end trace 0000000000000000 ]---
CFI_BACKWARD: saw 'call trace:|ok: control flow unchanged': ok

Thanks,
Dan.
diff mbox series

Patch

diff --git a/drivers/misc/lkdtm/cfi.c b/drivers/misc/lkdtm/cfi.c
index e88f778be0d5..59ba983ff23e 100644
--- a/drivers/misc/lkdtm/cfi.c
+++ b/drivers/misc/lkdtm/cfi.c
@@ -42,8 +42,138 @@  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
+
+/* 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(*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(*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