linux-kernel - Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Mar 2022 07:02:03 -0700
From:   Dan Li <ashimida@...ux.alibaba.com>
To:     catalin.marinas@....com, will@...nel.org, keescook@...omium.org,
        arnd@...db.de, gregkh@...uxfoundation.org, nathan@...nel.org,
        ndesaulniers@...gle.com, shuah@...nel.org, mark.rutland@....com,
        ojeda@...nel.org, akpm@...ux-foundation.org, elver@...gle.com,
        luc.vanoostenryck@...il.com, samitolvanen@...gle.com
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v4 2/2] lkdtm: Add Shadow Call Stack tests



On 3/14/22 06:53, Dan Li wrote:
> Add tests for SCS (Shadow Call Stack) based backward CFI.
> 
> +
> +#ifdef CONFIG_ARM64
> +/*
> + * This function is used to modify its return address. The PAC needs to be turned
> + * off here to ensure that the modification of the return address will not be blocked.
> + */
> +static noinline __no_ptrauth
> +void lkdtm_scs_set_lr(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;
> +}
> +
> +/* Function with __noscs attribute attempts to modify its return address. */
> +static noinline __no_ptrauth __noscs
> +void lkdtm_noscs_set_lr(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
> +static inline void lkdtm_noscs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +static inline void lkdtm_scs_set_lr(unsigned long *expected, unsigned long *addr) { }
> +#endif
> +
> +static volatile unsigned int force_label;
> +
> +/*
> + * This first checks whether a function with the __noscs attribute under
> + * the current platform can directly modify its return address, and if so,
> + * checks whether scs takes effect.
> + */
> +void __no_optimize lkdtm_CFI_BACKWARD_SHADOW(void)
> +{
> +	void *array[] = {&&unexpected, &&expected, &&good_scs, &&bad_scs};
> +
> +	if (force_label && (force_label < sizeof(array))) {
> +		/*
> +		 * Call them with "NULL" first to avoid
> +		 * arguments being treated as constants in -02.
> +		 */
> +		lkdtm_noscs_set_lr(NULL, NULL);
> +		lkdtm_scs_set_lr(NULL, NULL);
> +		goto *array[force_label];
> +	}
> +
> +	/* Keep labels in scope to avoid compiler warnings. */
> +	do {
> +		/* Verify the "normal" condition of LR corruption working. */
> +		pr_info("Trying to corrupt lr in a function without scs protection ...\n");
> +		lkdtm_noscs_set_lr(&&unexpected, &&expected);
> +
> +unexpected:
Hi, Kees,

With the default -O2, this code tests fine in gcc 11/clang 12, but
doesn't work in gcc 7.5.0. In 7.5.0, the generated code is as follows:

  bl      ffff8000088335c0 <lkdtm_noscs_set_lr>
  nop     						      ## return address of lkdtm_noscs_set_lr
  adrp    x0, ffff80000962b000 <kallsyms_token_index+0xe5908>  ## address of "&&unexpected"

The address of "&&unexpected" is still not guaranteed to always be the
same as the return address of lkdtm_noscs_set_lr, so I had to add
__no_optimize attribute here.

The code compiled under __no_optimize in above versions works fine, but
I saw the following description in the gcc user manual:

"You may not use this mechanism to jump to code in a different function.
If you do that, totally unpredictable things happen."

So there might be some risk here that we might not be able to run this
test case stably across all compiler versions, probably we still have to
use two test cases to complete this.

link: https://gcc.gnu.org/onlinedocs/gcc-11.2.0/gcc/Labels-as-Values.html#Labels-as-Values

Thanks,
Dan.

> +		/*
> +		 * If lr cannot be modified, the following check is meaningless,
> +		 * returns directly.
> +		 */
> +		pr_err("XPASS: Unexpectedly survived lr corruption without scs?!\n");
> +		break;
> +
> +expected:
> +		pr_info("ok: lr corruption redirected without scs.\n");
> +
> +		/* Verify that SCS is in effect. */
> +		pr_info("Trying to corrupt lr in a function with scs protection ...\n");
> +		lkdtm_scs_set_lr(&&good_scs, &&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);
> +}