| Message ID | 1576486038-9899-16-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: return address signing | expand |
On Mon, Dec 16, 2019 at 02:17:17PM +0530, Amit Daniel Kachhap wrote: > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 06b5025..f0798b7 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1466,6 +1466,7 @@ config ARM64_PTR_AUTH > bool "Enable support for pointer authentication" > default y > depends on !KVM || ARM64_VHE > + depends on GCC_VERSION >= 70000 || CLANG_VERSION >= 80000 Please don't add checks on compiler versions. Use cc-option when you need a certain option rather than guessing which compiler version supports it. People may do backports of different features, so the version is not relevant. > help > Pointer authentication (part of the ARMv8.3 Extensions) provides > instructions for signing and authenticating pointers against secret > @@ -1473,11 +1474,17 @@ 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. In this case, and if the target hardware is known to > + support pointer authentication, then CONFIG_STACKPROTECTOR can be > + disabled with minimal loss of protection. > + > The feature is detected at runtime. If the feature is not present in > hardware it will not be advertised to userspace/KVM guest nor will it > be enabled. However, KVM guest also require VHE mode and hence > @@ -1488,6 +1495,18 @@ config ARM64_PTR_AUTH > have address auth and the late CPU has then system panic will occur. > On such a system, this option should not be selected. > > +config CC_HAS_BRANCH_PROT_PAC_RET > + # GCC 9 or later, clang 8 or later > + def_bool $(cc-option,-mbranch-protection=pac-ret+leaf) > + > +config CC_HAS_SIGN_RETURN_ADDRESS > + # GCC 7, 8 > + def_bool $(cc-option,-msign-return-address=all) > + > +config AS_HAS_PAC > + def_bool $(as-option,-Wa,-march=armv8.3-a) > + depends on CC_IS_CLANG First, as I commented on the previous patch, clang seems to ignore -Wa, so you can write whatever you want after it and it seems to be always true. Second, if you need the assembler to support certain features, it needs to be checked irrespective of whether the compiler is gcc or clang. Binutils is a separate package. > + > endmenu > > config ARM64_SVE > diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile > index 1fbe24d..6a1da59 100644 > --- a/arch/arm64/Makefile > +++ b/arch/arm64/Makefile > @@ -72,6 +72,17 @@ stack_protector_prepare: prepare0 > include/generated/asm-offsets.h)) > endif > > +ifeq ($(CONFIG_ARM64_PTR_AUTH),y) > +branch-prot-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=all > +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf > +# -march=armv8.3-a enables the non-nops instructions for PAC, to avoid the compiler > +# to generate them and consequently to break the single image contract we pass it > +# only to the assembler when clang is selected as a compiler. For the GNU toolchain > +# this option is not used. > +branch-prot-flags-$(CONFIG_AS_HAS_PAC) += -Wa,-march=armv8.3-a > +KBUILD_CFLAGS += $(branch-prot-flags-y) > +endif So, does this actually work with clang? Please check the clang issue in case I'm mistaken. Otherwise, you could use as-instr as in this patch: https://lore.kernel.org/linux-arm-kernel/20200115113008.3334-3-catalin.marinas@arm.com/ Also Will had a preference for warning during build if the user requested a feature in .config (i.e. PAC) but the compiler/assembler does not support it (that was for the LSE patch above). You could attempt something similar with this patch.
On 1/17/20 11:49 AM, Catalin Marinas wrote: > On Mon, Dec 16, 2019 at 02:17:17PM +0530, Amit Daniel Kachhap wrote: >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index 06b5025..f0798b7 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -1466,6 +1466,7 @@ config ARM64_PTR_AUTH >> bool "Enable support for pointer authentication" >> default y >> depends on !KVM || ARM64_VHE >> + depends on GCC_VERSION >= 70000 || CLANG_VERSION >= 80000 > > Please don't add checks on compiler versions. Use cc-option when you > need a certain option rather than guessing which compiler version > supports it. People may do backports of different features, so the > version is not relevant. ok this is fixed locally. > >> help >> Pointer authentication (part of the ARMv8.3 Extensions) provides >> instructions for signing and authenticating pointers against secret >> @@ -1473,11 +1474,17 @@ 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. In this case, and if the target hardware is known to >> + support pointer authentication, then CONFIG_STACKPROTECTOR can be >> + disabled with minimal loss of protection. >> + >> The feature is detected at runtime. If the feature is not present in >> hardware it will not be advertised to userspace/KVM guest nor will it >> be enabled. However, KVM guest also require VHE mode and hence >> @@ -1488,6 +1495,18 @@ config ARM64_PTR_AUTH >> have address auth and the late CPU has then system panic will occur. >> On such a system, this option should not be selected. >> >> +config CC_HAS_BRANCH_PROT_PAC_RET >> + # GCC 9 or later, clang 8 or later >> + def_bool $(cc-option,-mbranch-protection=pac-ret+leaf) >> + >> +config CC_HAS_SIGN_RETURN_ADDRESS >> + # GCC 7, 8 >> + def_bool $(cc-option,-msign-return-address=all) >> + >> +config AS_HAS_PAC >> + def_bool $(as-option,-Wa,-march=armv8.3-a) >> + depends on CC_IS_CLANG > > First, as I commented on the previous patch, clang seems to ignore -Wa, > so you can write whatever you want after it and it seems to be always > true. > > Second, if you need the assembler to support certain features, it needs > to be checked irrespective of whether the compiler is gcc or clang. > Binutils is a separate package. ok agreed and done locally. > >> + >> endmenu >> >> config ARM64_SVE >> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile >> index 1fbe24d..6a1da59 100644 >> --- a/arch/arm64/Makefile >> +++ b/arch/arm64/Makefile >> @@ -72,6 +72,17 @@ stack_protector_prepare: prepare0 >> include/generated/asm-offsets.h)) >> endif >> >> +ifeq ($(CONFIG_ARM64_PTR_AUTH),y) >> +branch-prot-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=all >> +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf >> +# -march=armv8.3-a enables the non-nops instructions for PAC, to avoid the compiler >> +# to generate them and consequently to break the single image contract we pass it >> +# only to the assembler when clang is selected as a compiler. For the GNU toolchain >> +# this option is not used. >> +branch-prot-flags-$(CONFIG_AS_HAS_PAC) += -Wa,-march=armv8.3-a >> +KBUILD_CFLAGS += $(branch-prot-flags-y) >> +endif > > So, does this actually work with clang? Yes this works with clang. If I add -v to the KBUILD_CFLAGS then it splits the output and shows that the Wa arguments are given to the gcc assembler and clang compiler does not use it. > > Please check the clang issue in case I'm mistaken. Otherwise, you could > use as-instr as in this patch: > > https://lore.kernel.org/linux-arm-kernel/20200115113008.3334-3-catalin.marinas@arm.com/ > > Also Will had a preference for warning during build if the user > requested a feature in .config (i.e. PAC) but the compiler/assembler > does not support it (that was for the LSE patch above). You could > attempt something similar with this patch. I tried to add warnings like below which are in similar line to the above link, ifeq ($(CONFIG_ARM64_PTR_AUTH),y) + ifneq ($(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS),y) + ifneq ($(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET),y) +$(warning Pointer authentication not supported by compiler) + endif + endif + ifneq ($(CONFIG_AS_HAS_PAC),y) +$(warning Pointer authentication not supported by assembler) + endif endif But the issue is that warnings are printed twice and becomes confusing. First warning computed with the incorrect Kconfig flags and later with the correct computed Kconfig flags. This may be due to arch/arm64/Kconfig sourced twice. >
On 21/01/2020 14:37, Amit Kachhap wrote: [...] >> >> Also Will had a preference for warning during build if the user >> requested a feature in .config (i.e. PAC) but the compiler/assembler >> does not support it (that was for the LSE patch above). You could >> attempt something similar with this patch. > > I tried to add warnings like below which are in similar line to the above link, > > ifeq ($(CONFIG_ARM64_PTR_AUTH),y) > + ifneq ($(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS),y) > + ifneq ($(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET),y) > +$(warning Pointer authentication not supported by compiler) > + endif > + endif > + ifneq ($(CONFIG_AS_HAS_PAC),y) > +$(warning Pointer authentication not supported by assembler) > + endif > endif > > But the issue is that warnings are printed twice and becomes confusing. First > warning computed with the incorrect Kconfig flags and later with the correct > computed Kconfig flags. This may be due to arch/arm64/Kconfig sourced twice. > This seems similar to the problem we had on vDSOs some time ago. The main Makefile includes the arch specific twice, once for generating the building scripts and once for building the kernel. As a consequence of this the arch Kconfig is sourced twice. For the vDSOs we discarded the warning because it was causing confusion. > >>
On Tue, Jan 21, 2020 at 02:37:44PM +0000, Amit Kachhap wrote: > On 1/17/20 11:49 AM, Catalin Marinas wrote: > > Also Will had a preference for warning during build if the user > > requested a feature in .config (i.e. PAC) but the compiler/assembler > > does not support it (that was for the LSE patch above). You could > > attempt something similar with this patch. > > I tried to add warnings like below which are in similar line to the above > link, > > ifeq ($(CONFIG_ARM64_PTR_AUTH),y) > + ifneq ($(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS),y) > + ifneq ($(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET),y) > +$(warning Pointer authentication not supported by compiler) > + endif > + endif > + ifneq ($(CONFIG_AS_HAS_PAC),y) > +$(warning Pointer authentication not supported by assembler) > + endif > endif > > But the issue is that warnings are printed twice and becomes confusing. > First warning computed with the incorrect Kconfig flags and later with the > correct computed Kconfig flags. This may be due to arch/arm64/Kconfig > sourced twice. I think there are two passes over the opt arch Makefile, hence a warning based on the old config. Maybe there is a way to check which pass this is and skip the check in the first instance. It needs more digging, otherwise we could move the check to kernel/Makefile. Yet another option would be to check whether CONFIG_AS_HAS_PAC in .config is backed by the actual assembler option via the Makefile. It means we duplicate the as-option check but it may be the cleanest. Anyway, for the time being, just get the dependencies correctly in Kconfig without the warnings.
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 06b5025..f0798b7 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -1466,6 +1466,7 @@ config ARM64_PTR_AUTH bool "Enable support for pointer authentication" default y depends on !KVM || ARM64_VHE + depends on GCC_VERSION >= 70000 || CLANG_VERSION >= 80000 help Pointer authentication (part of the ARMv8.3 Extensions) provides instructions for signing and authenticating pointers against secret @@ -1473,11 +1474,17 @@ 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. In this case, and if the target hardware is known to + support pointer authentication, then CONFIG_STACKPROTECTOR can be + disabled with minimal loss of protection. + The feature is detected at runtime. If the feature is not present in hardware it will not be advertised to userspace/KVM guest nor will it be enabled. However, KVM guest also require VHE mode and hence @@ -1488,6 +1495,18 @@ config ARM64_PTR_AUTH have address auth and the late CPU has then system panic will occur. On such a system, this option should not be selected. +config CC_HAS_BRANCH_PROT_PAC_RET + # GCC 9 or later, clang 8 or later + def_bool $(cc-option,-mbranch-protection=pac-ret+leaf) + +config CC_HAS_SIGN_RETURN_ADDRESS + # GCC 7, 8 + def_bool $(cc-option,-msign-return-address=all) + +config AS_HAS_PAC + def_bool $(as-option,-Wa,-march=armv8.3-a) + depends on CC_IS_CLANG + endmenu config ARM64_SVE diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index 1fbe24d..6a1da59 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -72,6 +72,17 @@ stack_protector_prepare: prepare0 include/generated/asm-offsets.h)) endif +ifeq ($(CONFIG_ARM64_PTR_AUTH),y) +branch-prot-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=all +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf +# -march=armv8.3-a enables the non-nops instructions for PAC, to avoid the compiler +# to generate them and consequently to break the single image contract we pass it +# only to the assembler when clang is selected as a compiler. For the GNU toolchain +# this option is not used. +branch-prot-flags-$(CONFIG_AS_HAS_PAC) += -Wa,-march=armv8.3-a +KBUILD_CFLAGS += $(branch-prot-flags-y) +endif + ifeq ($(CONFIG_CPU_BIG_ENDIAN), y) KBUILD_CPPFLAGS += -mbig-endian CHECKFLAGS += -D__AARCH64EB__