| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <202203031010.0A492D114@keescook>
Date: Thu, 3 Mar 2022 10:42:45 -0800
From: Kees Cook <keescook@...omium.org>
To: Dan Li <ashimida@...ux.alibaba.com>
Cc: akpm@...ux-foundation.org, arnd@...db.de, catalin.marinas@....com,
gregkh@...uxfoundation.org, linux@...ck-us.net,
luc.vanoostenryck@...il.com, elver@...gle.com,
mark.rutland@....com, masahiroy@...nel.org, ojeda@...nel.org,
nathan@...nel.org, npiggin@...il.com, ndesaulniers@...gle.com,
samitolvanen@...gle.com, shuah@...nel.org, tglx@...utronix.de,
will@...nel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, linux-kselftest@...r.kernel.org,
llvm@...ts.linux.dev, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v3 2/2] lkdtm: Add Shadow Call Stack tests
On Wed, Mar 02, 2022 at 11:43:39PM -0800, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based
> backward CFI (as implemented by Clang and GCC).
Cool; thanks for writing these!
>
> Signed-off-by: Dan Li <ashimida@...ux.alibaba.com>
> ---
> drivers/misc/lkdtm/Makefile | 1 +
> drivers/misc/lkdtm/core.c | 2 +
> drivers/misc/lkdtm/lkdtm.h | 4 ++
> drivers/misc/lkdtm/scs.c | 67 +++++++++++++++++++++++++
> tools/testing/selftests/lkdtm/tests.txt | 2 +
> 5 files changed, 76 insertions(+)
> create mode 100644 drivers/misc/lkdtm/scs.c
>
> diff --git a/drivers/misc/lkdtm/Makefile b/drivers/misc/lkdtm/Makefile
> index 2e0aa74ac185..e2fb17868af2 100644
> --- a/drivers/misc/lkdtm/Makefile
> +++ b/drivers/misc/lkdtm/Makefile
> @@ -10,6 +10,7 @@ lkdtm-$(CONFIG_LKDTM) += rodata_objcopy.o
> lkdtm-$(CONFIG_LKDTM) += usercopy.o
> lkdtm-$(CONFIG_LKDTM) += stackleak.o
> lkdtm-$(CONFIG_LKDTM) += cfi.o
> +lkdtm-$(CONFIG_LKDTM) += scs.o
I'd expect these to be in cfi.c, rather than making a new source file.
> lkdtm-$(CONFIG_LKDTM) += fortify.o
> lkdtm-$(CONFIG_PPC_64S_HASH_MMU) += powerpc.o
>
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index f69b964b9952..d0ce0bec117c 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -178,6 +178,8 @@ static const struct crashtype crashtypes[] = {
> CRASHTYPE(USERCOPY_KERNEL),
> CRASHTYPE(STACKLEAK_ERASING),
> CRASHTYPE(CFI_FORWARD_PROTO),
> + CRASHTYPE(CFI_BACKWARD_SHADOW),
> + CRASHTYPE(CFI_BACKWARD_SHADOW_WITH_NOSCS),
> CRASHTYPE(FORTIFIED_OBJECT),
> CRASHTYPE(FORTIFIED_SUBOBJECT),
> CRASHTYPE(FORTIFIED_STRSCPY),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index d6137c70ebbe..a23d32dfc10b 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -158,6 +158,10 @@ void lkdtm_STACKLEAK_ERASING(void);
> /* cfi.c */
> void lkdtm_CFI_FORWARD_PROTO(void);
>
> +/* scs.c */
> +void lkdtm_CFI_BACKWARD_SHADOW(void);
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void);
> +
> /* fortify.c */
> void lkdtm_FORTIFIED_OBJECT(void);
> void lkdtm_FORTIFIED_SUBOBJECT(void);
> diff --git a/drivers/misc/lkdtm/scs.c b/drivers/misc/lkdtm/scs.c
> new file mode 100644
> index 000000000000..5922a55a8844
> --- /dev/null
> +++ b/drivers/misc/lkdtm/scs.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * This is for all the tests relating directly to Shadow Call Stack.
> + */
> +#include "lkdtm.h"
> +
> +#ifdef CONFIG_ARM64
> +/* Function clears its return address. */
> +static noinline void lkdtm_scs_clear_lr(void)
> +{
> + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
Is the asm needed here? Why not:
unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
*lr = 0;
> +}
> +
> +/* Function with __noscs attribute clears its return address. */
> +static noinline void __noscs lkdtm_noscs_clear_lr(void)
> +{
> + unsigned long *lr = (unsigned long *)__builtin_frame_address(0) + 1;
> +
> + asm volatile("str xzr, [%0]\n\t" : : "r"(lr) : "x30");
> +}
> +#endif
> +
> +/*
> + * This tries to call a function protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + * Due to the protection, the corruption will not take effect
> + * when the function returns.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW(void)
I think these two tests should be collapsed into a single one.
> +{
> +#ifdef CONFIG_ARM64
> + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> + return;
> + }
> +
> + pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> + lkdtm_scs_clear_lr();
> +
> + pr_err("ok: scs takes effect.\n");
> +#else
> + pr_err("XFAIL: this test is arm64-only\n");
> +#endif
This is slightly surprising -- we have no detection when a function has
its non-shadow-stack return address corrupted: it just _ignores_ the
value stored there. That seems like a missed opportunity for warning
about an unexpected state.
> +}
> +
> +/*
> + * This tries to call a function not protected by Shadow Call Stack,
> + * which corrupts its own return address during execution.
> + */
> +void lkdtm_CFI_BACKWARD_SHADOW_WITH_NOSCS(void)
> +{
> +#ifdef CONFIG_ARM64
> + if (!IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) {
> + pr_err("FAIL: kernel not built with CONFIG_SHADOW_CALL_STACK\n");
> + return;
Other tests try to give some hints about failures, e.g.:
pr_err("FAIL: cannot change for SCS\n");
pr_expected_config(CONFIG_SHADOW_CALL_STACK);
Though, having the IS_ENABLED in there makes me wonder if this test
should instead be made _survivable_ on failure. Something like this,
completely untested:
#ifdef CONFIG_ARM64
static noinline void lkdtm_scs_set_lr(unsigned long *addr)
{
unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
*lr = addr;
}
/* Function with __noscs attribute clears its return address. */
static noinline void __noscs lkdtm_noscs_set_lr(unsigned long *addr)
{
unsigned long **lr = (unsigned long **)__builtin_frame_address(0) + 1;
*lr = addr;
}
#endif
void lkdtm_CFI_BACKWARD_SHADOW(void)
{
#ifdef CONFIG_ARM64
/* Verify the "normal" condition of LR corruption working. */
do {
/* Keep label in scope to avoid compiler warning. */
if ((volatile int)0)
goto unexpected;
pr_info("Trying to corrupt lr in a function without scs protection ...\n");
lkdtm_noscs_set_lr(&&expected);
unexpected:
pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
break;
expected:
pr_err("ok: lr corruption redirected without scs.\n");
} while (0);
do {
/* Keep labe in scope to avoid compiler warning. */
if ((volatile int)0)
goto good_scs;
pr_info("Trying to corrupt lr in a function with scs protection ...\n");
lkdtm_scs_set_lr(&&bad_scs);
good_scs:
pr_info("ok: scs takes effect.\n");
break;
bad_scs:
pr_err("FAIL: return address rewritten!\n");
pr_expected_config(CONFIG_SHADOW_CALL_STACK);
} while (0);
#else
pr_err("XFAIL: this test is arm64-only\n");
#endif
}
And we should, actually, be able to make the "set_lr" functions be
arch-specific, leaving the test itself arch-agnostic....
--
Kees Cook
Powered by blists - more mailing lists