Re: [RFC v2 7/7] arm64: compile the kernel with ptrauth return address signing

From: Kees Cook <keescook@chromium.org>
To: Kristina Martsenko <kristina.martsenko@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
	Amit Kachhap <Amit.Kachhap@arm.com>,
	Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [RFC v2 7/7] arm64: compile the kernel with ptrauth return address signing
Date: Wed, 29 May 2019 20:45:22 -0700	[thread overview]
Message-ID: <201905292041.5297BF8C2F@keescook> (raw)
In-Reply-To: <20190529190332.29753-8-kristina.martsenko@arm.com>

On Wed, May 29, 2019 at 08:03:32PM +0100, Kristina Martsenko wrote:
> Compile all non-leaf functions with two ptrauth instructions: PACIASP in
> the prologue to sign the return address, and AUTIASP in the epilogue to
> authenticate the return address (from the stack). If authentication
> fails, the return will cause an instruction abort to be taken, followed
> by an oops and killing the task. This should help protect the kernel
> against attacks using return-oriented programming.
> 
> The new instructions are in the HINT encoding space, so on a system
> without ptrauth they execute as NOPs.
> 
> CONFIG_ARM64_PTR_AUTH now not only enables ptrauth for userspace and KVM
> guests, but also automatically builds the kernel with ptrauth
> instructions if the compiler supports it. If there is no compiler
> support, we do not warn that the kernel was built without ptrauth
> instructions.
> 
> GCC 7 and 8 support the -msign-return-address option, while GCC 9
> deprecates that option and replaces it with -mbranch-protection. Support
> both options.
> 
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>

Minor nits below...

Reviewed-by: Kees Cook <keescook@chromium.org>

> ---
> 
> Changes since RFC v1:
>  - Fixed support for compilers without ptrauth
>  - Added support for the new -mbranch-protection option
>  - Switched from protecting all functions to only protecting non-leaf functions
>    (for no good reason, I have not done e.g. gadget analysis)
>  - Moved __no_ptrauth definition to this patch, depending on compiler support
>  - Updated the Kconfig symbol description
>  - Updated the commit message
> 
>  arch/arm64/Kconfig                    | 12 +++++++++++-
>  arch/arm64/Makefile                   |  6 ++++++
>  arch/arm64/include/asm/pointer_auth.h |  6 ++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index f4c1e9b30129..3ce93d88fae1 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1295,11 +1295,15 @@ config ARM64_PTR_AUTH
>  	  and other attacks.
>  
>  	  This option enables these instructions at EL0 (i.e. for userspace).
> -
>  	  Choosing this option will cause the kernel to initialise secret keys
>  	  for each process at exec() time, with these keys being
>  	  context-switched along with the process.
>  
> +	  If the compiler supports the -mbranch-protection or
> +	  -msign-return-address flag (e.g. GCC 7 or later), then this option
> +	  will also cause the kernel itself to be compiled with return address
> +	  protection.
> +
>  	  The feature is detected at runtime. If the feature is not present in
>  	  hardware it will not be advertised to userspace nor will it be
>  	  enabled.
> @@ -1308,6 +1312,12 @@ config ARM64_PTR_AUTH
>  	  then the secondary CPU will be offlined. On such a system, this
>  	  option should not be selected.
>  
> +config CC_HAS_BRANCH_PROT_PAC_RET
> +	def_bool $(cc-option,-mbranch-protection=pac-ret)
> +
> +config CC_HAS_SIGN_RETURN_ADDRESS
> +	def_bool $(cc-option,-msign-return-address=non-leaf)
> +

I would add comments here for "GCC 9, Clang" and "GCC 7, 8"
respectively, just so it's quickly obvious what's to be expected when
reading this later. :)

>  endmenu
>  
>  config ARM64_SVE
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index b025304bde46..1dfbe755b531 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -66,6 +66,12 @@ stack_protector_prepare: prepare0
>  					include/generated/asm-offsets.h))
>  endif
>  
> +ifeq ($(CONFIG_ARM64_PTR_AUTH),y)
> +pac-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=non-leaf
> +pac-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret
> +KBUILD_CFLAGS += $(pac-flags-y)
> +endif
> +
>  ifeq ($(CONFIG_CPU_BIG_ENDIAN), y)
>  KBUILD_CPPFLAGS	+= -mbig-endian
>  CHECKFLAGS	+= -D__AARCH64EB__
> diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
> index 5491c34b4dc3..3a83c40ffd8a 100644
> --- a/arch/arm64/include/asm/pointer_auth.h
> +++ b/arch/arm64/include/asm/pointer_auth.h
> @@ -15,7 +15,13 @@
>   * allows pointer authentication to be enabled/disabled within the function
>   * (but leaves the function unprotected by pointer authentication).
>   */
> +#if defined(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET)
> +#define __no_ptrauth	__attribute__((target("branch-protection=none")))
> +#elif defined(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS)
> +#define __no_ptrauth	__attribute__((target("sign-return-address=none")))
> +#else
>  #define __no_ptrauth
> +#endif

Is arch/arm64/include/asm/pointer_auth.h going to be included always?
I suspect the correct place for this might end up being in
include/linux/compiler_types.h, but for now, only a few select places
need it, so this is probably fine as-is.

>  
>  /*
>   * Each key is a 128-bit quantity which is split across a pair of 64-bit
> -- 
> 2.11.0
> 

I'm excited to test this series! :)

-- 
Kees Cook

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel