[PATCH v2 00/10] arm64: BTI kernel and vDSO support

[PATCH v2 00/10] arm64: BTI kernel and vDSO support

[Mark Brown]

This patch series adds support for protecting the kernel and vDSO with
BTI including code compiled with the BPF JIT at runtime.

We build the kernel with annotations for BTI and then map the kernel
with GP based on the support on the boot CPU, rejecting secondaries that
don't have BTI support. If there is a need to handle big.LITTLE systems
with mismatched BTI support we will have to revisit this, currently no
such implementations exist.

I'm currently finalizing testing of the BPF support, the in-kernel
test_bpf tests run cleanly.

This series depends on:

 - for-next/bti in the arm64 tree
 - The series "arm64: Finish up assembler annotation modernisation"
 - The series "arm64: Make NOP handling a whitelist"

There is some discussion between Catalin and Will about the use of a
separate Kconfig option for this, I've left the separate option for now.

v2:
 - Enable support for building with GCC version 10 and later, a fix
   for BTI code generation is being backported to GCC 9 but is not yet
   available.
 - Add BPF support.
 - Remove some unused page attribute defines.
 - One assembler modernisation patch has been removed and sent
   separately.

Mark Brown (10):
  arm64: bti: Support building kernel C code using BTI
  arm64: asm: Override SYM_FUNC_START when building the kernel with BTI
  arm64: Set GP bit in kernel page tables to enable BTI for the kernel
  arm64: bpf: Annotate JITed code for BTI
  arm64: mm: Mark executable text as guarded pages
  arm64: bti: Provide Kconfig for kernel mode BTI
  arm64: asm: Provide a mechanism for generating ELF note for BTI
  arm64: vdso: Annotate for BTI
  arm64: vdso: Force the vDSO to be linked as BTI when built for BTI
  arm64: vdso: Map the vDSO text with guarded pages when built for BTI

 arch/arm64/Kconfig                    | 18 +++++++++++
 arch/arm64/Makefile                   |  4 +++
 arch/arm64/include/asm/assembler.h    | 41 ++++++++++++++++++++++++
 arch/arm64/include/asm/linkage.h      | 46 +++++++++++++++++++++++++++
 arch/arm64/include/asm/pgtable-prot.h |  3 ++
 arch/arm64/kernel/cpufeature.c        |  4 +++
 arch/arm64/kernel/vdso.c              |  6 +++-
 arch/arm64/kernel/vdso/Makefile       |  4 ++-
 arch/arm64/kernel/vdso/note.S         |  3 ++
 arch/arm64/kernel/vdso/sigreturn.S    |  3 ++
 arch/arm64/kernel/vdso/vdso.S         |  3 ++
 arch/arm64/mm/mmu.c                   | 24 ++++++++++++++
 arch/arm64/mm/pageattr.c              |  4 +--
 arch/arm64/net/bpf_jit.h              |  8 +++++
 arch/arm64/net/bpf_jit_comp.c         | 12 +++++++
 15 files changed, 179 insertions(+), 4 deletions(-)

-- 
2.20.1


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

[PATCH v2 01/10] arm64: bti: Support building kernel C code using BTI

[Mark Brown]

When running with BTI enabled we need to ask the compiler to enable
generation of BTI landing pads beyond those generated as a result of
pointer authentication instructions being landing pads. Since the two
features are practically speaking unlikely to be used separately we
will make kernel mode BTI depend on pointer authentication in order
to simplify the Makefile.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
index 85e4149cc5d5..90150b5b180e 100644
--- a/arch/arm64/Makefile
+++ b/arch/arm64/Makefile
@@ -71,7 +71,11 @@ branch-prot-flags-y += $(call cc-option,-mbranch-protection=none)
 
 ifeq ($(CONFIG_ARM64_PTR_AUTH),y)
 branch-prot-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=all
+ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
+branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
+else
 branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
+endif
 # -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. This option is utilized only in case of non
-- 
2.20.1


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

[PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

When the kernel is built for BTI override SYM_FUNC_START and related macros
to add a BTI landing pad to the start of all global functions, ensuring that
they are BTI safe. The ; at the end of the BTI_C macro is for the benefit of
the macro-generated functions in xen-hypercall.S.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/linkage.h | 46 ++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm64/include/asm/linkage.h b/arch/arm64/include/asm/linkage.h
index ebee3113a62f..d8d5b0f77216 100644
--- a/arch/arm64/include/asm/linkage.h
+++ b/arch/arm64/include/asm/linkage.h
@@ -4,6 +4,52 @@
 #define __ALIGN		.align 2
 #define __ALIGN_STR	".align 2"
 
+#ifdef CONFIG_ARM64_BTI_KERNEL
+
+/*
+ * Since current versions of gas reject the BTI instruction unless we
+ * set the architecture version to v8.5 we use the hint instruction
+ * instead.
+ */
+#define BTI_C hint 34 ;
+#define BTI_J hint 36 ;
+
+/*
+ * When using in-kernel BTI we need to ensure that assembly functions
+ * have suitable annotations.  Override SYM_FUNC_START to insert a BTI
+ * landing pad at the start of everything.
+ */
+#define SYM_FUNC_START(name)				\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)	\
+	BTI_C
+
+#define SYM_FUNC_START_NOALIGN(name)			\
+	SYM_START(name, SYM_L_GLOBAL, SYM_A_NONE)	\
+	BTI_C
+
+#define SYM_FUNC_START_LOCAL(name)			\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_ALIGN)	\
+	BTI_C
+
+#define SYM_FUNC_START_LOCAL_NOALIGN(name)		\
+	SYM_START(name, SYM_L_LOCAL, SYM_A_NONE)	\
+	BTI_C
+
+#define SYM_FUNC_START_WEAK(name)			\
+	SYM_START(name, SYM_L_WEAK, SYM_A_ALIGN)	\
+	BTI_C
+
+#define SYM_FUNC_START_WEAK_NOALIGN(name)		\
+	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
+	BTI_C
+
+#define SYM_INNER_LABEL(name, linkage)			\
+	.type name SYM_T_NONE ASM_NL			\
+	SYM_ENTRY(name, linkage, SYM_A_NONE)		\
+	BTI_J
+
+#endif
+
 /*
  * Annotate a function as position independent, i.e., safe to be called before
  * the kernel virtual mapping is activated.
-- 
2.20.1


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

[PATCH v2 03/10] arm64: Set GP bit in kernel page tables to enable BTI for the kernel

[Mark Brown]

Now that the kernel is built with BTI annotations enable the feature by
setting the GP bit in the stage 1 translation tables.  This is done
based on the features supported by the boot CPU so that we do not need
to rewrite the translation tables.

In order to avoid potential issues on big.LITTLE systems when there are
a mix of BTI and non-BTI capable CPUs in the system when we have enabled
kernel mode BTI we change BTI to be a _STRICT_BOOT_CPU_FEATURE when we
have kernel BTI.  This will prevent any CPUs that don't support BTI
being started if the boot CPU supports BTI rather than simply not using
BTI as we do when supporting BTI only in userspace.  The main concern is
the possibility of BTYPE being preserved by a CPU that does not
implement BTI when a thread is migrated to it resulting in an incorrect
state which could generate an exception when the thread migrates back to
a CPU that does support BTI.  If we encounter practical systems which
mix BTI and non-BTI CPUs we will need to revisit this implementation.

Since we currently do not generate landing pads in the BPF JIT we only
map the base kernel text in this way.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/pgtable-prot.h |  3 +++
 arch/arm64/kernel/cpufeature.c        |  4 ++++
 arch/arm64/mm/mmu.c                   | 24 ++++++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 1305e28225fc..310690332896 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -21,6 +21,7 @@
 
 #ifndef __ASSEMBLY__
 
+#include <asm/cpufeature.h>
 #include <asm/pgtable-types.h>
 
 extern bool arm64_use_ng_mappings;
@@ -31,6 +32,8 @@ extern bool arm64_use_ng_mappings;
 #define PTE_MAYBE_NG		(arm64_use_ng_mappings ? PTE_NG : 0)
 #define PMD_MAYBE_NG		(arm64_use_ng_mappings ? PMD_SECT_NG : 0)
 
+#define PTE_MAYBE_GP		(system_supports_bti() ? PTE_GP : 0)
+
 #define PROT_DEFAULT		(_PROT_DEFAULT | PTE_MAYBE_NG)
 #define PROT_SECT_DEFAULT	(_PROT_SECT_DEFAULT | PMD_MAYBE_NG)
 
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9793d3aa9d98..84fea674856f 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1800,7 +1800,11 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
 	{
 		.desc = "Branch Target Identification",
 		.capability = ARM64_BTI,
+#ifdef CONFIG_ARM64_BTI_KERNEL
+		.type = ARM64_CPUCAP_STRICT_BOOT_CPU_FEATURE,
+#else
 		.type = ARM64_CPUCAP_SYSTEM_FEATURE,
+#endif
 		.matches = has_cpuid_feature,
 		.cpu_enable = bti_enable,
 		.sys_reg = SYS_ID_AA64PFR1_EL1,
diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index a374e4f51a62..c299b73dd5e4 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -609,6 +609,22 @@ static int __init map_entry_trampoline(void)
 core_initcall(map_entry_trampoline);
 #endif
 
+/*
+ * Open coded check for BTI, only for use to determine configuration
+ * for early mappings for before the cpufeature code has run.
+ */
+static bool arm64_early_this_cpu_has_bti(void)
+{
+	u64 pfr1;
+
+	if (!IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
+		return false;
+
+	pfr1 = read_sysreg_s(SYS_ID_AA64PFR1_EL1);
+	return cpuid_feature_extract_unsigned_field(pfr1,
+						    ID_AA64PFR1_BT_SHIFT);
+}
+
 /*
  * Create fine-grained mappings for the kernel.
  */
@@ -624,6 +640,14 @@ static void __init map_kernel(pgd_t *pgdp)
 	 */
 	pgprot_t text_prot = rodata_enabled ? PAGE_KERNEL_ROX : PAGE_KERNEL_EXEC;
 
+	/*
+	 * If we have a CPU that supports BTI and a kernel built for
+	 * BTI then mark the kernel executable text as guarded pages
+	 * now so we don't have to rewrite the page tables later.
+	 */
+	if (arm64_early_this_cpu_has_bti())
+		text_prot = __pgprot_modify(text_prot, PTE_GP, PTE_GP);
+
 	/*
 	 * Only rodata will be remapped with different permissions later on,
 	 * all other segments are allowed to use contiguous mappings.
-- 
2.20.1


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

[PATCH v2 04/10] arm64: bpf: Annotate JITed code for BTI

[Mark Brown]

In order to extend the protection offered by BTI to all code executing in
kernel mode we need to annotate JITed BPF code appropriately for BTI. To
do this we need to add a landing pad to the start of each BPF function and
also immediately after the function prologue if we are emitting a function
which can be tail called. Jumps within BPF functions are all to immediate
offsets and therefore do not require landing pads.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/net/bpf_jit.h      |  8 ++++++++
 arch/arm64/net/bpf_jit_comp.c | 12 ++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/arm64/net/bpf_jit.h b/arch/arm64/net/bpf_jit.h
index eb73f9f72c46..05b477709b5f 100644
--- a/arch/arm64/net/bpf_jit.h
+++ b/arch/arm64/net/bpf_jit.h
@@ -189,4 +189,12 @@
 /* Rn & Rm; set condition flags */
 #define A64_TST(sf, Rn, Rm) A64_ANDS(sf, A64_ZR, Rn, Rm)
 
+/* HINTs */
+#define A64_HINT(x) aarch64_insn_gen_hint(x)
+
+/* BTI */
+#define A64_BTI_C  A64_HINT(AARCH64_INSN_HINT_BTIC)
+#define A64_BTI_J  A64_HINT(AARCH64_INSN_HINT_BTIJ)
+#define A64_BTI_JC A64_HINT(AARCH64_INSN_HINT_BTIJC)
+
 #endif /* _BPF_JIT_H */
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c
index cdc79de0c794..83fa475c6b42 100644
--- a/arch/arm64/net/bpf_jit_comp.c
+++ b/arch/arm64/net/bpf_jit_comp.c
@@ -171,7 +171,11 @@ static inline int epilogue_offset(const struct jit_ctx *ctx)
 #define STACK_ALIGN(sz) (((sz) + 15) & ~15)
 
 /* Tail call offset to jump into */
+#if IS_ENABLED(CONFIG_ARM64_BTI_KERNEL)
+#define PROLOGUE_OFFSET 8
+#else
 #define PROLOGUE_OFFSET 7
+#endif
 
 static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
 {
@@ -208,6 +212,10 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
 	 *
 	 */
 
+	/* BTI landing pad */
+	if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
+		emit(A64_BTI_C, ctx);
+
 	/* Save FP and LR registers to stay align with ARM64 AAPCS */
 	emit(A64_PUSH(A64_FP, A64_LR, A64_SP), ctx);
 	emit(A64_MOV(1, A64_FP, A64_SP), ctx);
@@ -230,6 +238,10 @@ static int build_prologue(struct jit_ctx *ctx, bool ebpf_from_cbpf)
 				    cur_offset, PROLOGUE_OFFSET);
 			return -1;
 		}
+
+		/* BTI landing pad for the tail call, done with a BR */
+		if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL))
+			emit(A64_BTI_J, ctx);
 	}
 
 	ctx->stack_size = STACK_ALIGN(prog->aux->stack_depth);
-- 
2.20.1


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

[PATCH v2 05/10] arm64: mm: Mark executable text as guarded pages

[Mark Brown]

When the kernel is built for BTI and running on a system which supports
it make all executable text guarded pages to ensure that loadable module
and JITed BPF code is protected by BTI.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/mm/pageattr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
index 250c49008d73..bde08090b838 100644
--- a/arch/arm64/mm/pageattr.c
+++ b/arch/arm64/mm/pageattr.c
@@ -126,13 +126,13 @@ int set_memory_nx(unsigned long addr, int numpages)
 {
 	return change_memory_common(addr, numpages,
 					__pgprot(PTE_PXN),
-					__pgprot(0));
+					__pgprot(PTE_MAYBE_GP));
 }
 
 int set_memory_x(unsigned long addr, int numpages)
 {
 	return change_memory_common(addr, numpages,
-					__pgprot(0),
+					__pgprot(PTE_MAYBE_GP),
 					__pgprot(PTE_PXN));
 }
 
-- 
2.20.1


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

[PATCH v2 06/10] arm64: bti: Provide Kconfig for kernel mode BTI

[Mark Brown]

Now that all the code is in place provide a Kconfig option allowing users
to enable BTI for the kernel if their toolchain supports it, defaulting it
on since this has security benefits. This is a separate configuration
option since we currently don't support secondary CPUs that lack BTI if
the boot CPU supports it.

Code generation issues mean that current GCC 9 versions are not able to
produce usable BTI binaries so we disable support for building with GCC
versions prior to 10, once a fix is backported to GCC 9 the dependencies
will be updated.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/Kconfig | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6f199d8146d4..f3de1c115fc0 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1610,6 +1610,24 @@ config ARM64_BTI
 	  BTI, such binaries can still run, but you get no additional
 	  enforcement of branch destinations.
 
+config ARM64_BTI_KERNEL
+	bool "Use Branch Target Identification for kernel"
+	default y
+	depends on ARM64_BTI
+	depends on ARM64_PTR_AUTH
+	depends on CC_HAS_BRANCH_PROT_PAC_RET_BTI
+	depends on !CC_IS_GCC || GCC_VERSION >= 100000
+	depends on (!FUNCTION_GRAPH_TRACER || DYNAMIC_FTRACE_WITH_REGS)
+	help
+	  Build the kernel with Branch Target Identification annotations
+	  and enable enforcement of this for kernel code. When this option
+	  is enabled and the system supports BTI all kernel code including
+	  modular code must have BTI enabled.
+
+config CC_HAS_BRANCH_PROT_PAC_RET_BTI
+	# GCC 9 or later, clang 8 or later
+	def_bool $(cc-option,-mbranch-protection=pac-ret+leaf+bti)
+
 config ARM64_E0PD
 	bool "Enable support for E0PD"
 	default y
-- 
2.20.1


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

[PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Mark Brown]

ELF files built for BTI should have a program property note section which
identifies them as such. The linker expects to find this note in all
object files it is linking into a BTI annotated output, the compiler will
ensure that this happens for C files but for assembler files we need to do
this in the source so provide a macro which can be used for this purpose.

This is mainly for use in the VDSO which should be a normal ELF shared
library and should therefore include BTI annotations when built for BTI.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 0bff325117b4..85a88df2d0fe 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -736,4 +736,45 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 .Lyield_out_\@ :
 	.endm
 
+/*
+ * This macro emits a program property note section identifying
+ * architecture features which require special handling, mainly for
+ * use in assembly files included in the VDSO.
+ */
+
+#ifdef CONFIG_ARM64_BTI_KERNEL
+
+#define NT_GNU_PROPERTY_TYPE_0  5
+#define GNU_PROPERTY_AARCH64_FEATURE_1_AND      0xc0000000
+
+#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
+#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)
+
+.macro emit_aarch64_feature_1_and
+	.pushsection .note.gnu.property, "a"
+	.align  3
+	.long   2f - 1f
+	.long   6f - 3f
+	.long   NT_GNU_PROPERTY_TYPE_0
+1:      .string "GNU"
+2:
+	.align  3
+3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
+	.long   5f - 4f
+4:
+	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
+		GNU_PROPERTY_AARCH64_FEATURE_1_BTI
+5:
+	.align  3
+6:
+	.popsection
+.endm
+
+#else
+
+.macro emit_aarch64_feature_1_and
+.endm
+
+#endif  /* CONFIG_ARM64_BTI_KERNEL */
+
 #endif	/* __ASM_ASSEMBLER_H */
-- 
2.20.1


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

[PATCH v2 08/10] arm64: vdso: Annotate for BTI

[Mark Brown]

Generate BTI annotations for all assembly files included in the 64 bit
vDSO.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/vdso/note.S      | 3 +++
 arch/arm64/kernel/vdso/sigreturn.S | 3 +++
 arch/arm64/kernel/vdso/vdso.S      | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/arch/arm64/kernel/vdso/note.S b/arch/arm64/kernel/vdso/note.S
index 0ce6ec75a525..3d4e82290c80 100644
--- a/arch/arm64/kernel/vdso/note.S
+++ b/arch/arm64/kernel/vdso/note.S
@@ -12,9 +12,12 @@
 #include <linux/version.h>
 #include <linux/elfnote.h>
 #include <linux/build-salt.h>
+#include <asm/assembler.h>
 
 ELFNOTE_START(Linux, 0, "a")
 	.long LINUX_VERSION_CODE
 ELFNOTE_END
 
 BUILD_SALT
+
+emit_aarch64_feature_1_and
diff --git a/arch/arm64/kernel/vdso/sigreturn.S b/arch/arm64/kernel/vdso/sigreturn.S
index 12324863d5c2..3fb13b81f780 100644
--- a/arch/arm64/kernel/vdso/sigreturn.S
+++ b/arch/arm64/kernel/vdso/sigreturn.S
@@ -9,6 +9,7 @@
  */
 
 #include <linux/linkage.h>
+#include <asm/assembler.h>
 #include <asm/unistd.h>
 
 	.text
@@ -24,3 +25,5 @@ SYM_FUNC_START(__kernel_rt_sigreturn)
 	svc	#0
 	.cfi_endproc
 SYM_FUNC_END(__kernel_rt_sigreturn)
+
+emit_aarch64_feature_1_and
diff --git a/arch/arm64/kernel/vdso/vdso.S b/arch/arm64/kernel/vdso/vdso.S
index d1414fee5274..c4b1990bf2be 100644
--- a/arch/arm64/kernel/vdso/vdso.S
+++ b/arch/arm64/kernel/vdso/vdso.S
@@ -8,6 +8,7 @@
 #include <linux/init.h>
 #include <linux/linkage.h>
 #include <linux/const.h>
+#include <asm/assembler.h>
 #include <asm/page.h>
 
 	.globl vdso_start, vdso_end
@@ -19,3 +20,5 @@ vdso_start:
 vdso_end:
 
 	.previous
+
+emit_aarch64_feature_1_and
-- 
2.20.1


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

[PATCH v2 09/10] arm64: vdso: Force the vDSO to be linked as BTI when built for BTI

[Mark Brown]

When the kernel and hence vDSO are built with BTI enabled force the linker
to link the vDSO as BTI. This will cause the linker to warn if any of the
input files do not have the BTI annotation, ensuring that we don't silently
fail to provide a vDSO that is built and annotated for BTI when the
kernel is being built with BTI.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/vdso/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/vdso/Makefile b/arch/arm64/kernel/vdso/Makefile
index dd2514bb1511..51ad1cce8133 100644
--- a/arch/arm64/kernel/vdso/Makefile
+++ b/arch/arm64/kernel/vdso/Makefile
@@ -17,8 +17,10 @@ obj-vdso := vgettimeofday.o note.o sigreturn.o
 targets := $(obj-vdso) vdso.so vdso.so.dbg
 obj-vdso := $(addprefix $(obj)/, $(obj-vdso))
 
+btildflags-$(CONFIG_ARM64_BTI_KERNEL) += -z force-bti
+
 ldflags-y := -shared -nostdlib -soname=linux-vdso.so.1 --hash-style=sysv \
-		--build-id -n -T
+		--build-id -n $(btildflags-y) -T
 
 ccflags-y := -fno-common -fno-builtin -fno-stack-protector -ffixed-x18
 ccflags-y += -DDISABLE_BRANCH_PROFILING
-- 
2.20.1


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

[PATCH v2 10/10] arm64: vdso: Map the vDSO text with guarded pages when built for BTI

[Mark Brown]

The kernel is responsible for mapping the vDSO into userspace processes,
including mapping the text section as executable. Handle the mapping of
the vDSO for BTI similarly, mapping the text section as guarded pages so
the BTI annotations in the vDSO become effective when they are present.

This will mean that we can have BTI active for the vDSO in processes that
do not otherwise support BTI. This should not be an issue for any expected
use of the vDSO.

Signed-off-by: Mark Brown <broonie@kernel.org>
---
 arch/arm64/kernel/vdso.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/vdso.c b/arch/arm64/kernel/vdso.c
index 354b11e27c07..9271b7774df0 100644
--- a/arch/arm64/kernel/vdso.c
+++ b/arch/arm64/kernel/vdso.c
@@ -142,6 +142,7 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 				    int uses_interp)
 {
 	unsigned long vdso_base, vdso_text_len, vdso_mapping_len;
+	unsigned long gp_flags = 0;
 	void *ret;
 
 	vdso_text_len = vdso_lookup[arch_index].vdso_pages << PAGE_SHIFT;
@@ -160,10 +161,13 @@ static int __setup_additional_pages(enum arch_vdso_type arch_index,
 	if (IS_ERR(ret))
 		goto up_fail;
 
+	if (IS_ENABLED(CONFIG_ARM64_BTI_KERNEL) && system_supports_bti())
+		gp_flags = VM_ARM64_BTI;
+
 	vdso_base += PAGE_SIZE;
 	mm->context.vdso = (void *)vdso_base;
 	ret = _install_special_mapping(mm, vdso_base, vdso_text_len,
-				       VM_READ|VM_EXEC|
+				       VM_READ|VM_EXEC|gp_flags|
 				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
 				       vdso_lookup[arch_index].cm);
 	if (IS_ERR(ret))
-- 
2.20.1


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

Re: [PATCH v2 00/10] arm64: BTI kernel and vDSO support

[Catalin Marinas]

On Wed, Apr 29, 2020 at 10:16:31PM +0100, Mark Brown wrote:
> We build the kernel with annotations for BTI and then map the kernel
> with GP based on the support on the boot CPU, rejecting secondaries that
> don't have BTI support. If there is a need to handle big.LITTLE systems
> with mismatched BTI support we will have to revisit this, currently no
> such implementations exist.

It's fine by me to live with this assumption.

> I'm currently finalizing testing of the BPF support, the in-kernel
> test_bpf tests run cleanly.
> 
> This series depends on:
> 
>  - for-next/bti in the arm64 tree
>  - The series "arm64: Finish up assembler annotation modernisation"
>  - The series "arm64: Make NOP handling a whitelist"
> 
> There is some discussion between Catalin and Will about the use of a
> separate Kconfig option for this, I've left the separate option for now.

Leave them separated, we may do the same for PAC.

We could allow the vdso to use BTI while the kernel option is disabled
but I don't think it's worth the hassle.

> v2:
>  - Enable support for building with GCC version 10 and later, a fix
>    for BTI code generation is being backported to GCC 9 but is not yet
>    available.

Do you have a link to a GCC commit or bugzilla? (for future reference,
no need to update the patch).

The series looks fine to me.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

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

Re: [PATCH v2 00/10] arm64: BTI kernel and vDSO support

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 641 bytes --]

On Thu, Apr 30, 2020 at 06:18:43PM +0100, Catalin Marinas wrote:
> On Wed, Apr 29, 2020 at 10:16:31PM +0100, Mark Brown wrote:

> > v2:
> >  - Enable support for building with GCC version 10 and later, a fix
> >    for BTI code generation is being backported to GCC 9 but is not yet
> >    available.

> Do you have a link to a GCC commit or bugzilla? (for future reference,
> no need to update the patch).

I found these:

	https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94697
	https://gcc.gnu.org/pipermail/gcc-patches/2020-April/544429.html

> The series looks fine to me.

> Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Thanks.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Wed, Apr 29, 2020 at 10:16:33PM +0100, Mark Brown wrote:
> +#define SYM_FUNC_START(name)				\
> +	SYM_START(name, SYM_L_GLOBAL, SYM_A_ALIGN)	\
> +	BTI_C
> +
> +#define SYM_FUNC_START_NOALIGN(name)			\
> +	SYM_START(name, SYM_L_GLOBAL, SYM_A_NONE)	\
> +	BTI_C
> +
> +#define SYM_FUNC_START_LOCAL(name)			\
> +	SYM_START(name, SYM_L_LOCAL, SYM_A_ALIGN)	\
> +	BTI_C
> +
> +#define SYM_FUNC_START_LOCAL_NOALIGN(name)		\
> +	SYM_START(name, SYM_L_LOCAL, SYM_A_NONE)	\
> +	BTI_C
> +
> +#define SYM_FUNC_START_WEAK(name)			\
> +	SYM_START(name, SYM_L_WEAK, SYM_A_ALIGN)	\
> +	BTI_C
> +
> +#define SYM_FUNC_START_WEAK_NOALIGN(name)		\
> +	SYM_START(name, SYM_L_WEAK, SYM_A_NONE)		\
> +	BTI_C
> +
> +#define SYM_INNER_LABEL(name, linkage)			\
> +	.type name SYM_T_NONE ASM_NL			\
> +	SYM_ENTRY(name, linkage, SYM_A_NONE)		\
> +	BTI_J

This break building the compat vDSO:

arch/arm64/kernel/vdso32/sigreturn.S: Assembler messages:
arch/arm64/kernel/vdso32/sigreturn.S:20: Error: bad instruction `hint 34'
arch/arm64/kernel/vdso32/sigreturn.S:30: Error: bad instruction `hint 34'
arch/arm64/kernel/vdso32/sigreturn.S:41: Error: bad instruction `hint 34'
arch/arm64/kernel/vdso32/sigreturn.S:51: Error: bad instruction `hint 34'
clang-10: error: assembler command failed with exit code 1 (use -v to see invocation)

I also see a scary linker warning about the native VDSO:

aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.

Will

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Will Deacon]

[+Dave]

On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote:
> ELF files built for BTI should have a program property note section which
> identifies them as such. The linker expects to find this note in all
> object files it is linking into a BTI annotated output, the compiler will
> ensure that this happens for C files but for assembler files we need to do
> this in the source so provide a macro which can be used for this purpose.
> 
> This is mainly for use in the VDSO which should be a normal ELF shared
> library and should therefore include BTI annotations when built for BTI.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 0bff325117b4..85a88df2d0fe 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -736,4 +736,45 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
>  .Lyield_out_\@ :
>  	.endm
>  
> +/*
> + * This macro emits a program property note section identifying
> + * architecture features which require special handling, mainly for
> + * use in assembly files included in the VDSO.
> + */
> +
> +#ifdef CONFIG_ARM64_BTI_KERNEL
> +
> +#define NT_GNU_PROPERTY_TYPE_0  5
> +#define GNU_PROPERTY_AARCH64_FEATURE_1_AND      0xc0000000
> +
> +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
> +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)
> +
> +.macro emit_aarch64_feature_1_and

Might be useful to take the features as a macro argument, so we can
re-use this when extra features get added in the future.

> +	.pushsection .note.gnu.property, "a"
> +	.align  3
> +	.long   2f - 1f
> +	.long   6f - 3f
> +	.long   NT_GNU_PROPERTY_TYPE_0
> +1:      .string "GNU"
> +2:
> +	.align  3
> +3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
> +	.long   5f - 4f
> +4:
> +	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
> +		GNU_PROPERTY_AARCH64_FEATURE_1_BTI

Hmm. The Linux ABI doc [1] says this field is:

	unsigned char pr_data[PR_DATASZ];

but the AArch64 PCS [2] says:

	"It has a single 32-bit value for the pr_data field."

What does this mean for endianness?

Will

[1] https://github.com/hjl-tools/linux-abi/wiki/linux-abi-draft.pdf
[2] https://static.docs.arm.com/ihi0056/g/aaelf64.pdf

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1142 bytes --]

On Tue, May 05, 2020 at 03:56:42PM +0100, Will Deacon wrote:
> On Wed, Apr 29, 2020 at 10:16:33PM +0100, Mark Brown wrote:

> > +#define SYM_INNER_LABEL(name, linkage)			\
> > +	.type name SYM_T_NONE ASM_NL			\
> > +	SYM_ENTRY(name, linkage, SYM_A_NONE)		\
> > +	BTI_J

> This break building the compat vDSO:

Right, fixed locally.

> I also see a scary linker warning about the native VDSO:

> aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.

I can't reproduce this, I am using clang-10 as well...  can you share
your exact command line and config?  I'm using

    make -j56 ARCH=arm64 CROSS_COMPILE=aarch64-none-linux-gnu- CC=clang-10

which is the same clang version as you, not that I'd expect that to make
a difference.  This looks like the C code isn't being built with BTI
enabled but I can't see how you're managing to do that and I'm not -
this is the warning that "arm64: vdso: Force the VDSO to be linked as
BTI when built for BTI" is intended to trigger if something goes wrong
so I guess it's a good job I enabled that :/

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Tue, May 05, 2020 at 04:18:06PM +0100, Mark Brown wrote:
> On Tue, May 05, 2020 at 03:56:42PM +0100, Will Deacon wrote:
> > On Wed, Apr 29, 2020 at 10:16:33PM +0100, Mark Brown wrote:
> 
> > > +#define SYM_INNER_LABEL(name, linkage)			\
> > > +	.type name SYM_T_NONE ASM_NL			\
> > > +	SYM_ENTRY(name, linkage, SYM_A_NONE)		\
> > > +	BTI_J
> 
> > This break building the compat vDSO:
> 
> Right, fixed locally.

Thanks!

> > I also see a scary linker warning about the native VDSO:
> 
> > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> 
> I can't reproduce this, I am using clang-10 as well...  can you share
> your exact command line and config?  I'm using
> 
>     make -j56 ARCH=arm64 CROSS_COMPILE=aarch64-none-linux-gnu- CC=clang-10
> 
> which is the same clang version as you, not that I'd expect that to make
> a difference.  This looks like the C code isn't being built with BTI
> enabled but I can't see how you're managing to do that and I'm not -
> this is the warning that "arm64: vdso: Force the VDSO to be linked as
> BTI when built for BTI" is intended to trigger if something goes wrong
> so I guess it's a good job I enabled that :/

Just been debugging this, and it seems that not all clangs are created
equal. Updating mine from 10.0.2 to 11.0.1 means I now get the
'.note.gnu.property' sections emitted for C files compiled using
'-mbranch-protection=pac-ret+leaf+bti', whereas I didn't before.

Cheers,

Will

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

Re: [PATCH v2 01/10] arm64: bti: Support building kernel C code using BTI

[Dave Martin]

On Wed, Apr 29, 2020 at 10:16:32PM +0100, Mark Brown wrote:
> When running with BTI enabled we need to ask the compiler to enable
> generation of BTI landing pads beyond those generated as a result of
> pointer authentication instructions being landing pads. Since the two
> features are practically speaking unlikely to be used separately we
> will make kernel mode BTI depend on pointer authentication in order
> to simplify the Makefile.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile
> index 85e4149cc5d5..90150b5b180e 100644
> --- a/arch/arm64/Makefile
> +++ b/arch/arm64/Makefile
> @@ -71,7 +71,11 @@ branch-prot-flags-y += $(call cc-option,-mbranch-protection=none)
>  
>  ifeq ($(CONFIG_ARM64_PTR_AUTH),y)
>  branch-prot-flags-$(CONFIG_CC_HAS_SIGN_RETURN_ADDRESS) := -msign-return-address=all
> +ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> +else
>  branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> +endif

Is it worth a comment to explain how this differs from
-mbranch-protection=standard, and why we're not using that here?

Cheers
---Dave

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Dave Martin]

On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote:
> [+Dave]
> 
> On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote:
> > ELF files built for BTI should have a program property note section which
> > identifies them as such. The linker expects to find this note in all
> > object files it is linking into a BTI annotated output, the compiler will
> > ensure that this happens for C files but for assembler files we need to do
> > this in the source so provide a macro which can be used for this purpose.
> > 
> > This is mainly for use in the VDSO which should be a normal ELF shared
> > library and should therefore include BTI annotations when built for BTI.
> > 
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >  arch/arm64/include/asm/assembler.h | 41 ++++++++++++++++++++++++++++++
> >  1 file changed, 41 insertions(+)
> > 
> > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> > index 0bff325117b4..85a88df2d0fe 100644
> > --- a/arch/arm64/include/asm/assembler.h
> > +++ b/arch/arm64/include/asm/assembler.h
> > @@ -736,4 +736,45 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
> >  .Lyield_out_\@ :
> >  	.endm
> >  
> > +/*
> > + * This macro emits a program property note section identifying
> > + * architecture features which require special handling, mainly for
> > + * use in assembly files included in the VDSO.
> > + */
> > +
> > +#ifdef CONFIG_ARM64_BTI_KERNEL
> > +
> > +#define NT_GNU_PROPERTY_TYPE_0  5
> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_AND      0xc0000000
> > +
> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)
> > +
> > +.macro emit_aarch64_feature_1_and
> 
> Might be useful to take the features as a macro argument, so we can
> re-use this when extra features get added in the future.

Probably a good idea, though I hope this doesn't crop up too often...

> > +	.pushsection .note.gnu.property, "a"
> > +	.align  3
> > +	.long   2f - 1f
> > +	.long   6f - 3f
> > +	.long   NT_GNU_PROPERTY_TYPE_0
> > +1:      .string "GNU"
> > +2:
> > +	.align  3
> > +3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
> > +	.long   5f - 4f
> > +4:
> > +	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
> > +		GNU_PROPERTY_AARCH64_FEATURE_1_BTI
> 
> Hmm. The Linux ABI doc [1] says this field is:
> 
> 	unsigned char pr_data[PR_DATASZ];
> 
> but the AArch64 PCS [2] says:
> 
> 	"It has a single 32-bit value for the pr_data field."
> 
> What does this mean for endianness?

I think this means it's poorly specified.

The spirit of this is that each property is a container for a random ELF
structure, whose elements are encoded with endianness matching that of 
the ELF file.

Because these structures are variably sized, they can't be described
properly using a C type.  The pseudo-C in [1] is illustrative but a bit
of a bodge IMHO.  Attempting to do things this way for real would also
get you into trouble with strict-aliasing IIUC.


This ship has already sailed: someone should check what comes out of
cc -mbig-endian -mbranch-protection=standard.  (Extra points scored if
you can persuade gcc and clang to do something different!)

Cheers
---Dave

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1814 bytes --]

On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote:
> On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote:

> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)

> > +.macro emit_aarch64_feature_1_and

> Might be useful to take the features as a macro argument, so we can
> re-use this when extra features get added in the future.

I was unsure about that - it'd be a bit annoying to have to have all the
callers of the macro list things like BTI where 

> > +3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
> > +	.long   5f - 4f
> > +4:
> > +	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
> > +		GNU_PROPERTY_AARCH64_FEATURE_1_BTI

> Hmm. The Linux ABI doc [1] says this field is:

> 	unsigned char pr_data[PR_DATASZ];

> but the AArch64 PCS [2] says:

> 	"It has a single 32-bit value for the pr_data field."

> What does this mean for endianness?

It's not entirely clear is it?  What we're doing here means that we're
emitting as a long rather than a character array so the endianness
matters.  The ABI doc does have language about the elements being "an
array of X-byte integers in the format of the target processor" which
seems to align with that as well, my impression is that the intention of
the ABI doc is that there should be a Processor_Word type corresponding
to the Elf_Word type but there isn't so the char arrays are used to
handle the word size difference between AArch32 and AArch64.

Unless I'm missing something this at least appears to agree with what
the compilers (both GCC and clang) are emitting for both big and little
endian and what a readelf that understands these is decoding so I think
at this point it's de facto the way things are interpreted.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1183 bytes --]

On Tue, May 05, 2020 at 05:08:53PM +0100, Will Deacon wrote:
> On Tue, May 05, 2020 at 04:18:06PM +0100, Mark Brown wrote:

> > > I also see a scary linker warning about the native VDSO:

> > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.

> > I can't reproduce this, I am using clang-10 as well...  can you share
> > your exact command line and config?  I'm using

> Just been debugging this, and it seems that not all clangs are created
> equal. Updating mine from 10.0.2 to 11.0.1 means I now get the
> '.note.gnu.property' sections emitted for C files compiled using
> '-mbranch-protection=pac-ret+leaf+bti', whereas I didn't before.

Oh, that's irritating.  I'm using clang from the llvm.org packages,
currently that's:

    10.0.0-++20200115115127+cbe681bd833-1~exp1~20200115105727.528

so if this is a clang issue it looks like they fixed it in the clang-10
branch.  I'm not sure it's worth trying to detect and handle this or
not, I don't know how widely deployed toolchains that don't emit the
property are and there's a fairly clear solution.  What do you think?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 01/10] arm64: bti: Support building kernel C code using BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1261 bytes --]

On Tue, May 05, 2020 at 05:50:45PM +0100, Dave Martin wrote:
> On Wed, Apr 29, 2020 at 10:16:32PM +0100, Mark Brown wrote:

> > +ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
> > +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
> > +else
> >  branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
> > +endif

> Is it worth a comment to explain how this differs from
> -mbranch-protection=standard, and why we're not using that here?

There was some open discussion in a separate thread about the use of
+leaf:

   https://lore.kernel.org/linux-arm-kernel/1588149371-20310-1-git-send-email-amit.kachhap@arm.com/

which currently settled on leaving it in but I do agree, it probably
would be useful to document the choices we're making here.  CCing Amit
as some of the discussion was off-list so I don't know all the details
there.

As with the recent change to explicitly default to branch-protection=none 
there's probably a case for simply being explicit about whatever we're
doing rather than relying on compiler defaults in case there are any bad
interactions with code generated for an extension that isn't fully
supported in the kernel in the future.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:
> On Tue, May 05, 2020 at 05:08:53PM +0100, Will Deacon wrote:
> > On Tue, May 05, 2020 at 04:18:06PM +0100, Mark Brown wrote:
> 
> > > > I also see a scary linker warning about the native VDSO:
> 
> > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> 
> > > I can't reproduce this, I am using clang-10 as well...  can you share
> > > your exact command line and config?  I'm using
> 
> > Just been debugging this, and it seems that not all clangs are created
> > equal. Updating mine from 10.0.2 to 11.0.1 means I now get the
> > '.note.gnu.property' sections emitted for C files compiled using
> > '-mbranch-protection=pac-ret+leaf+bti', whereas I didn't before.
> 
> Oh, that's irritating.  I'm using clang from the llvm.org packages,
> currently that's:
> 
>     10.0.0-++20200115115127+cbe681bd833-1~exp1~20200115105727.528
> 
> so if this is a clang issue it looks like they fixed it in the clang-10
> branch.  I'm not sure it's worth trying to detect and handle this or
> not, I don't know how widely deployed toolchains that don't emit the
> property are and there's a fairly clear solution.  What do you think?

The linker fixes this up when it warns, right? If so, I think the current
behaviour is fine *but* we might want to improve the diagnostic a bit not
to worry/confuse people. e.g. something like:

  "Your compiler is not emitting '.note.gnu.property' sections: forcing
   support for BTI in the linker, but consider upgrading your toolchain."

Will

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1525 bytes --]

On Wed, May 06, 2020 at 08:10:26AM +0100, Will Deacon wrote:
> On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:

> > > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.

> > so if this is a clang issue it looks like they fixed it in the clang-10
> > branch.  I'm not sure it's worth trying to detect and handle this or
> > not, I don't know how widely deployed toolchains that don't emit the
> > property are and there's a fairly clear solution.  What do you think?

> The linker fixes this up when it warns, right? If so, I think the current

The linker is fixing this up, yes.

> behaviour is fine *but* we might want to improve the diagnostic a bit not
> to worry/confuse people. e.g. something like:

>   "Your compiler is not emitting '.note.gnu.property' sections: forcing
>    support for BTI in the linker, but consider upgrading your toolchain."

Well, the theory behind the warning is that if the compiler is emitting
code suitable for the features described in the note then it should
always emit the appropriate annotations so the warning is more intended
to be telling the user that the code is trying to link in code that's
not built properly and will most likely fail at runtime.  In the current
situation that's an issue with the toolchain not emitting the
annotations but the common case expectation is that the issue will be
that there are object files that weren't built appropriately.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:
> On Wed, May 06, 2020 at 08:10:26AM +0100, Will Deacon wrote:
> > On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:
> 
> > > > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> 
> > > so if this is a clang issue it looks like they fixed it in the clang-10
> > > branch.  I'm not sure it's worth trying to detect and handle this or
> > > not, I don't know how widely deployed toolchains that don't emit the
> > > property are and there's a fairly clear solution.  What do you think?
> 
> > The linker fixes this up when it warns, right? If so, I think the current
> 
> The linker is fixing this up, yes.
> 
> > behaviour is fine *but* we might want to improve the diagnostic a bit not
> > to worry/confuse people. e.g. something like:
> 
> >   "Your compiler is not emitting '.note.gnu.property' sections: forcing
> >    support for BTI in the linker, but consider upgrading your toolchain."
> 
> Well, the theory behind the warning is that if the compiler is emitting
> code suitable for the features described in the note then it should
> always emit the appropriate annotations so the warning is more intended
> to be telling the user that the code is trying to link in code that's
> not built properly and will most likely fail at runtime.  In the current
> situation that's an issue with the toolchain not emitting the
> annotations but the common case expectation is that the issue will be
> that there are object files that weren't built appropriately.

Hmm, I suppose, although it's a bit belt-and-braces given that we've got
the right options in KBUILD_CFLAGS. What about:

	"Your compiler is not emitting '.note.gnu.property' sections: forcing
	 support for BTI in the linker, but check your CFLAGS and consider
	 upgrading your toolchain."

I'd usually not be too bothered, but having run into this yesterday and
not understood the problem, I'd like to save somebody else from puzzling
over this if we can!

Will

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Will Deacon]

Hi Mark, Dave,

On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote:
> On Tue, May 05, 2020 at 03:58:59PM +0100, Will Deacon wrote:
> > On Wed, Apr 29, 2020 at 10:16:38PM +0100, Mark Brown wrote:
> 
> > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
> > > +#define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)
> 
> > > +.macro emit_aarch64_feature_1_and
> 
> > Might be useful to take the features as a macro argument, so we can
> > re-use this when extra features get added in the future.
> 
> I was unsure about that - it'd be a bit annoying to have to have all the
> callers of the macro list things like BTI where 

It just feels inevitable that we'll need to do this at some point!
Do you know what is supposed to happen if an object has multiple instances
of this property identifying different features? For example, could we
do something like:

	emit_aarch64_feature_1_and_pac_bti
	emit_aarch64_feature_1_and_whizz
	emit_aarch64_feature_1_and_bang

all of which wrap emit_aarch64_feature_1_and, but result in an object that
supports pac, bti, whizz and bang?

If we have to merge this stuff in a single .long, then I think we'll
probably have to put up with passing in the features as an optional macro
argument, which defaults to "all features" if it's omitted. So on top of
your patches, we could do:


diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 85a88df2d0fe..53801250a639 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -750,7 +750,11 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 #define GNU_PROPERTY_AARCH64_FEATURE_1_BTI      (1U << 0)
 #define GNU_PROPERTY_AARCH64_FEATURE_1_PAC      (1U << 1)
 
-.macro emit_aarch64_feature_1_and
+#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL				\
+				(GNU_PROPERTY_AARCH64_FEATURE_1_BTI |	\
+				 GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
+
+.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL
 	.pushsection .note.gnu.property, "a"
 	.align  3
 	.long   2f - 1f
@@ -762,8 +766,13 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
 	.long   5f - 4f
 4:
-	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
-		GNU_PROPERTY_AARCH64_FEATURE_1_BTI
+	/*
+	 * Although the Linux ABI spec describes this as an array of
+	 * unsigned char, the rest of the world (including clang and gcc)
+	 * treat it as a 32-bit value and so no swizzling is required
+	 * when building for big-endian.
+	 */
+	.long   \feat
 5:
 	.align  3
 6:
@@ -772,7 +781,7 @@ USER(\label, ic	ivau, \tmp2)			// invalidate I line PoU
 
 #else
 
-.macro emit_aarch64_feature_1_and
+.macro emit_aarch64_feature_1_and, feat=0
 .endm
 
 #endif  /* CONFIG_ARM64_BTI_KERNEL */


> > > +3:      .long   GNU_PROPERTY_AARCH64_FEATURE_1_AND
> > > +	.long   5f - 4f
> > > +4:
> > > +	.long   GNU_PROPERTY_AARCH64_FEATURE_1_PAC | \
> > > +		GNU_PROPERTY_AARCH64_FEATURE_1_BTI
> 
> > Hmm. The Linux ABI doc [1] says this field is:
> 
> > 	unsigned char pr_data[PR_DATASZ];
> 
> > but the AArch64 PCS [2] says:
> 
> > 	"It has a single 32-bit value for the pr_data field."
> 
> > What does this mean for endianness?
> 
> It's not entirely clear is it?  What we're doing here means that we're
> emitting as a long rather than a character array so the endianness
> matters.  The ABI doc does have language about the elements being "an
> array of X-byte integers in the format of the target processor" which
> seems to align with that as well, my impression is that the intention of
> the ABI doc is that there should be a Processor_Word type corresponding
> to the Elf_Word type but there isn't so the char arrays are used to
> handle the word size difference between AArch32 and AArch64.
> 
> Unless I'm missing something this at least appears to agree with what
> the compilers (both GCC and clang) are emitting for both big and little
> endian and what a readelf that understands these is decoding so I think
> at this point it's de facto the way things are interpreted.

As long as the compilers agree with each other and with the way we roll the
note ourself, then I think all we should do is add a comment above the
.long. Sound reasonable?

Will

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2125 bytes --]

On Wed, May 06, 2020 at 11:50:06AM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:

> > Well, the theory behind the warning is that if the compiler is emitting
> > code suitable for the features described in the note then it should
> > always emit the appropriate annotations so the warning is more intended
> > to be telling the user that the code is trying to link in code that's
> > not built properly and will most likely fail at runtime.  In the current

> Hmm, I suppose, although it's a bit belt-and-braces given that we've got
> the right options in KBUILD_CFLAGS. What about:

We know we're doing fine but the warning is being emitted by the linker
rather than something in the kernel tree and it doesn't know what the
users are or that they invoked their compilers correctly.

> 	"Your compiler is not emitting '.note.gnu.property' sections: forcing
> 	 support for BTI in the linker, but check your CFLAGS and consider
> 	 upgrading your toolchain."

> I'd usually not be too bothered, but having run into this yesterday and
> not understood the problem, I'd like to save somebody else from puzzling
> over this if we can!

That's a bit C specific - CFLAGS isn't going to apply to other
languages and binutils could be linking pretty much anything.  TBH I'd
expect one of the common cases would be assembler given the pain
involved in generating notes.  Possibly "build configuration and
toolchain" instead but that's also pretty vague so might not help much
with people being confused?  Either way any diagnostic change would be a
binutils change, I've flagged this up to the toolchain people
internally.

I do think that this will be a lot easier going forwards - hopefully the
problem with the toolchain not generating notes is not going to be an
issue by the time people are actively deploying BTI (people using GCC
are going to need a shiny new version anyway and I don't know how
widespread the clang versions that have issues are), and if people do
start running into it then it'll be possible to usefully search for the
error message online which should help a lot.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 01/10] arm64: bti: Support building kernel C code using BTI

[Amit Kachhap]

Hi,

On 5/5/20 11:01 PM, Mark Brown wrote:
> On Tue, May 05, 2020 at 05:50:45PM +0100, Dave Martin wrote:
>> On Wed, Apr 29, 2020 at 10:16:32PM +0100, Mark Brown wrote:
> 
>>> +ifeq ($(CONFIG_ARM64_BTI_KERNEL),y)
>>> +branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET_BTI) := -mbranch-protection=pac-ret+leaf+bti
>>> +else
>>>   branch-prot-flags-$(CONFIG_CC_HAS_BRANCH_PROT_PAC_RET) := -mbranch-protection=pac-ret+leaf
>>> +endif
> 
>> Is it worth a comment to explain how this differs from
>> -mbranch-protection=standard, and why we're not using that here?
> 
> There was some open discussion in a separate thread about the use of
> +leaf:
> 
>     https://lore.kernel.org/linux-arm-kernel/1588149371-20310-1-git-send-email-amit.kachhap@arm.com/
> 
> which currently settled on leaving it in but I do agree, it probably
> would be useful to document the choices we're making here.  CCing Amit
> as some of the discussion was off-list so I don't know all the details
> there.

It was decided to retain the strictest ptrauth compiler option as there
is a narrow scope of ROP protection use-case in case of leaf function.
Although I provided some static benefits in diluting this requirement
but it was suggested to differ it till there is some run-time
performance gain data for leaf functions.

Yes documenting it here makes sense.

Cheers,
Amit Daniel

> 
> As with the recent change to explicitly default to branch-protection=none
> there's probably a case for simply being explicit about whatever we're
> doing rather than relying on compiler defaults in case there are any bad
> interactions with code generated for an extension that isn't fully
> supported in the kernel in the future.
> 

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Wed, May 06, 2020 at 12:43:53PM +0100, Mark Brown wrote:
> On Wed, May 06, 2020 at 11:50:06AM +0100, Will Deacon wrote:
> > On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:
> 
> > > Well, the theory behind the warning is that if the compiler is emitting
> > > code suitable for the features described in the note then it should
> > > always emit the appropriate annotations so the warning is more intended
> > > to be telling the user that the code is trying to link in code that's
> > > not built properly and will most likely fail at runtime.  In the current
> 
> > Hmm, I suppose, although it's a bit belt-and-braces given that we've got
> > the right options in KBUILD_CFLAGS. What about:
> 
> We know we're doing fine but the warning is being emitted by the linker
> rather than something in the kernel tree and it doesn't know what the
> users are or that they invoked their compilers correctly.
> 
> > 	"Your compiler is not emitting '.note.gnu.property' sections: forcing
> > 	 support for BTI in the linker, but check your CFLAGS and consider
> > 	 upgrading your toolchain."
> 
> > I'd usually not be too bothered, but having run into this yesterday and
> > not understood the problem, I'd like to save somebody else from puzzling
> > over this if we can!
> 
> That's a bit C specific - CFLAGS isn't going to apply to other
> languages and binutils could be linking pretty much anything.  TBH I'd
> expect one of the common cases would be assembler given the pain
> involved in generating notes.  Possibly "build configuration and
> toolchain" instead but that's also pretty vague so might not help much
> with people being confused?  Either way any diagnostic change would be a
> binutils change, I've flagged this up to the toolchain people
> internally.

Ah, that's where we're talking past each other: I was only thinking of
printing this from the Makefile after seeing the warning from ld. I
completely agree that the stuff I was suggesting is kernel-specific, and
sorry for not being clear.

That said, without a way for the linker to force bti *without* generating
the warning, I'm not sure how we could implement this sensibly. The warning
also seems only to be generated if some objects have the BTI property and
others don't; if all objects are missing it then it's quiet.

Maybe we could detect that the compiler doesn't generate the property
section, and then avoid generating them in our assembly files? i.e.
wrap '.macro emit_aarch64_feature_1_and' in a #ifdef CC_HAS_GNU_PROPERTY
... #endif block?

> I do think that this will be a lot easier going forwards - hopefully the
> problem with the toolchain not generating notes is not going to be an
> issue by the time people are actively deploying BTI (people using GCC
> are going to need a shiny new version anyway and I don't know how
> widespread the clang versions that have issues are), and if people do
> start running into it then it'll be possible to usefully search for the
> error message online which should help a lot.

I think we'll get reports of people running into this (I hit it with a
defconfig build), so just looking for an idea of what we might do if/when
that happens.

Will

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1607 bytes --]

On Wed, May 06, 2020 at 12:26:33PM +0100, Will Deacon wrote:
> On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote:

> > I was unsure about that - it'd be a bit annoying to have to have all the
> > callers of the macro list things like BTI where 

> It just feels inevitable that we'll need to do this at some point!
> Do you know what is supposed to happen if an object has multiple instances
> of this property identifying different features? For example, could we
> do something like:

Regardless of what is supposed to happen my strong suspicion is that
we'll have some more.

> If we have to merge this stuff in a single .long, then I think we'll
> probably have to put up with passing in the features as an optional macro
> argument, which defaults to "all features" if it's omitted. So on top of
> your patches, we could do:

> +#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL				\
> +				(GNU_PROPERTY_AARCH64_FEATURE_1_BTI |	\
> +				 GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
> +
> +.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL

Right, I was just expecting to have the ifdefs selecting the flags to
emit in the middle of the asm macro definiton rather than separately - I
didn't see a huge win in defining a macro with the only user being
another macro.  I can do something along those lines though.

> -.macro emit_aarch64_feature_1_and
> +.macro emit_aarch64_feature_1_and, feat=0
>  .endm

That will result in us emitting the note with no flags set which
*should* be totally fine but is a bit unusual and feels like tempting
fate.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 2560 bytes --]

On Wed, May 06, 2020 at 01:27:10PM +0100, Will Deacon wrote:

> That said, without a way for the linker to force bti *without* generating
> the warning, I'm not sure how we could implement this sensibly. The warning
> also seems only to be generated if some objects have the BTI property and
> others don't; if all objects are missing it then it's quiet.

Oh, that's annoying - I'd expected the warning to be generated if any of
the inputs miss the BTI annotation.  Part of my goal with turning it on 
was to provide some defensiveness against future breakage which could
potentially include messing something up with enabling BTI for the
inputs.  I'll tell the toolchain people about that too :/

> Maybe we could detect that the compiler doesn't generate the property
> section, and then avoid generating them in our assembly files? i.e.
> wrap '.macro emit_aarch64_feature_1_and' in a #ifdef CC_HAS_GNU_PROPERTY
> ... #endif block?

If we're going to do this detection it might be better to just disable
kernel BTI support entirely if the toolchain doesn't emit the notes,
treating the missing notes as a most likely overcautious warning flag
that there might be code generation bugs as well.  Either way it does
feel like a lot of work.

> > I do think that this will be a lot easier going forwards - hopefully the
> > problem with the toolchain not generating notes is not going to be an
> > issue by the time people are actively deploying BTI (people using GCC
> > are going to need a shiny new version anyway and I don't know how
> > widespread the clang versions that have issues are), and if people do
> > start running into it then it'll be possible to usefully search for the
> > error message online which should help a lot.

> I think we'll get reports of people running into this (I hit it with a
> defconfig build), so just looking for an idea of what we might do if/when
> that happens.

Bear in mind that to use BTI kernel support you need to be using either
clang or version 10 or later of GCC (which hasn't yet been released,
it's almost there) so it's not going to be triggering in the common case
where people are using released GCC versions.  The change in clang that
you're missing is:

   https://reviews.llvm.org/rGd53e61863d48a07ce285d5b0a36abc67583023bd

which is from December last year so rather recent meaning I do think
it's a valid concern for clang, I'm just not sure how widespread clang
usage is with people who don't also update their toolchains with a
fairly high frequency.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Dave Martin]

On Wed, May 06, 2020 at 11:50:06AM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:
> > On Wed, May 06, 2020 at 08:10:26AM +0100, Will Deacon wrote:
> > > On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:
> > 
> > > > > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> > 
> > > > so if this is a clang issue it looks like they fixed it in the clang-10
> > > > branch.  I'm not sure it's worth trying to detect and handle this or
> > > > not, I don't know how widely deployed toolchains that don't emit the
> > > > property are and there's a fairly clear solution.  What do you think?
> > 
> > > The linker fixes this up when it warns, right? If so, I think the current
> > 
> > The linker is fixing this up, yes.
> > 
> > > behaviour is fine *but* we might want to improve the diagnostic a bit not
> > > to worry/confuse people. e.g. something like:
> > 
> > >   "Your compiler is not emitting '.note.gnu.property' sections: forcing
> > >    support for BTI in the linker, but consider upgrading your toolchain."
> > 
> > Well, the theory behind the warning is that if the compiler is emitting
> > code suitable for the features described in the note then it should
> > always emit the appropriate annotations so the warning is more intended
> > to be telling the user that the code is trying to link in code that's
> > not built properly and will most likely fail at runtime.  In the current
> > situation that's an issue with the toolchain not emitting the
> > annotations but the common case expectation is that the issue will be
> > that there are object files that weren't built appropriately.
> 
> Hmm, I suppose, although it's a bit belt-and-braces given that we've got
> the right options in KBUILD_CFLAGS. What about:
> 
> 	"Your compiler is not emitting '.note.gnu.property' sections: forcing
> 	 support for BTI in the linker, but check your CFLAGS and consider
> 	 upgrading your toolchain."
> 
> I'd usually not be too bothered, but having run into this yesterday and
> not understood the problem, I'd like to save somebody else from puzzling
> over this if we can!

I really don't think we should fudge this: if the linker doesn't think
the inputs are BTI-enabled then the compiler or linker is broken, or
there's a bug in the kernel source tree.

The checking done by the toolchain is important -- if we want to
suppress it, we should have an override option than depends on BROKEN
(because yes, you're explicitly risking a broken kernel if you do this).

The fact that all gcc and clang both screwed this up in various ways at
some point is not our fault, or our problem, providing that fixes are
available...

Am I being too paranoid?

Cheers
---Dave

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Will Deacon]

On Wed, May 06, 2020 at 01:38:55PM +0100, Mark Brown wrote:
> On Wed, May 06, 2020 at 12:26:33PM +0100, Will Deacon wrote:
> > On Tue, May 05, 2020 at 06:06:29PM +0100, Mark Brown wrote:
> 
> > > I was unsure about that - it'd be a bit annoying to have to have all the
> > > callers of the macro list things like BTI where 
> 
> > It just feels inevitable that we'll need to do this at some point!
> > Do you know what is supposed to happen if an object has multiple instances
> > of this property identifying different features? For example, could we
> > do something like:
> 
> Regardless of what is supposed to happen my strong suspicion is that
> we'll have some more.

Yup! :)

> > If we have to merge this stuff in a single .long, then I think we'll
> > probably have to put up with passing in the features as an optional macro
> > argument, which defaults to "all features" if it's omitted. So on top of
> > your patches, we could do:
> 
> > +#define GNU_PROPERTY_AARCH64_FEATURE_1_ALL				\
> > +				(GNU_PROPERTY_AARCH64_FEATURE_1_BTI |	\
> > +				 GNU_PROPERTY_AARCH64_FEATURE_1_PAC)
> > +
> > +.macro emit_aarch64_feature_1_and, feat=GNU_PROPERTY_AARCH64_FEATURE_1_ALL
> 
> Right, I was just expecting to have the ifdefs selecting the flags to
> emit in the middle of the asm macro definiton rather than separately - I
> didn't see a huge win in defining a macro with the only user being
> another macro.  I can do something along those lines though.

With my suggestion, we still only have the 'emit_aarch64_feature_1_and'
macro, it just provides a way to override the properties if we need that
later on. All I'm proposing is adding the optional feat parameter, which
defaults to all of the properties we know about.

> > -.macro emit_aarch64_feature_1_and
> > +.macro emit_aarch64_feature_1_and, feat=0
> >  .endm
> 
> That will result in us emitting the note with no flags set which
> *should* be totally fine but is a bit unusual and feels like tempting
> fate.

Nah, that's just the dummy .macro definition.

Will

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Wed, May 06, 2020 at 02:40:21PM +0100, Dave Martin wrote:
> On Wed, May 06, 2020 at 11:50:06AM +0100, Will Deacon wrote:
> > On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:
> > > On Wed, May 06, 2020 at 08:10:26AM +0100, Will Deacon wrote:
> > > > On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:
> > > 
> > > > > > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> > > 
> > > > > so if this is a clang issue it looks like they fixed it in the clang-10
> > > > > branch.  I'm not sure it's worth trying to detect and handle this or
> > > > > not, I don't know how widely deployed toolchains that don't emit the
> > > > > property are and there's a fairly clear solution.  What do you think?
> > > 
> > > > The linker fixes this up when it warns, right? If so, I think the current
> > > 
> > > The linker is fixing this up, yes.
> > > 
> > > > behaviour is fine *but* we might want to improve the diagnostic a bit not
> > > > to worry/confuse people. e.g. something like:
> > > 
> > > >   "Your compiler is not emitting '.note.gnu.property' sections: forcing
> > > >    support for BTI in the linker, but consider upgrading your toolchain."
> > > 
> > > Well, the theory behind the warning is that if the compiler is emitting
> > > code suitable for the features described in the note then it should
> > > always emit the appropriate annotations so the warning is more intended
> > > to be telling the user that the code is trying to link in code that's
> > > not built properly and will most likely fail at runtime.  In the current
> > > situation that's an issue with the toolchain not emitting the
> > > annotations but the common case expectation is that the issue will be
> > > that there are object files that weren't built appropriately.
> > 
> > Hmm, I suppose, although it's a bit belt-and-braces given that we've got
> > the right options in KBUILD_CFLAGS. What about:
> > 
> > 	"Your compiler is not emitting '.note.gnu.property' sections: forcing
> > 	 support for BTI in the linker, but check your CFLAGS and consider
> > 	 upgrading your toolchain."
> > 
> > I'd usually not be too bothered, but having run into this yesterday and
> > not understood the problem, I'd like to save somebody else from puzzling
> > over this if we can!
> 
> I really don't think we should fudge this: if the linker doesn't think
> the inputs are BTI-enabled then the compiler or linker is broken, or
> there's a bug in the kernel source tree.
> 
> The checking done by the toolchain is important -- if we want to
> suppress it, we should have an override option than depends on BROKEN
> (because yes, you're explicitly risking a broken kernel if you do this).
> 
> The fact that all gcc and clang both screwed this up in various ways at
> some point is not our fault, or our problem, providing that fixes are
> available...
> 
> Am I being too paranoid?

I don't think so, but I'm just looking for an answer to "What do we do if
people start running into this warning?". As it stands, it sounds like it's
unlikely that they will, but if they do then we're going to have to hack
something to make it go away.

Will

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1596 bytes --]

On Wed, May 06, 2020 at 03:45:44PM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 02:40:21PM +0100, Dave Martin wrote:

> > I really don't think we should fudge this: if the linker doesn't think
> > the inputs are BTI-enabled then the compiler or linker is broken, or
> > there's a bug in the kernel source tree.

> > The checking done by the toolchain is important -- if we want to
> > suppress it, we should have an override option than depends on BROKEN
> > (because yes, you're explicitly risking a broken kernel if you do this).

> > The fact that all gcc and clang both screwed this up in various ways at
> > some point is not our fault, or our problem, providing that fixes are
> > available...

> > Am I being too paranoid?

That's my position too, we want the warning - I think if we're going to
do any handling it should either be to prevent use of kernel BTI
entirely or to do what Will was originally suggesting and print some
explanatory text somewhere.  My inclination is that the former is safer
and fits more with the general architecture approach to this sort of
thing.

> I don't think so, but I'm just looking for an answer to "What do we do if
> people start running into this warning?". As it stands, it sounds like it's
> unlikely that they will, but if they do then we're going to have to hack
> something to make it go away.

It's possible that this e-mail thread showing up in searches might well
answer people's questions anyway (it is going to be one of the few hits
for the warning ATM).  How about we deal with this when we get to it, or
at least as a followup?

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Dave Martin]

On Wed, May 06, 2020 at 03:45:44PM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 02:40:21PM +0100, Dave Martin wrote:
> > On Wed, May 06, 2020 at 11:50:06AM +0100, Will Deacon wrote:
> > > On Wed, May 06, 2020 at 11:41:52AM +0100, Mark Brown wrote:
> > > > On Wed, May 06, 2020 at 08:10:26AM +0100, Will Deacon wrote:
> > > > > On Tue, May 05, 2020 at 06:21:00PM +0100, Mark Brown wrote:
> > > > 
> > > > > > > > > aarch64-none-linux-gnu-ld: arch/arm64/kernel/vdso/vgettimeofday.o: warning: BTI turned on by -z force-bti when all inputs do not have BTI in NOTE section.
> > > > 
> > > > > > so if this is a clang issue it looks like they fixed it in the clang-10
> > > > > > branch.  I'm not sure it's worth trying to detect and handle this or
> > > > > > not, I don't know how widely deployed toolchains that don't emit the
> > > > > > property are and there's a fairly clear solution.  What do you think?
> > > > 
> > > > > The linker fixes this up when it warns, right? If so, I think the current
> > > > 
> > > > The linker is fixing this up, yes.
> > > > 
> > > > > behaviour is fine *but* we might want to improve the diagnostic a bit not
> > > > > to worry/confuse people. e.g. something like:
> > > > 
> > > > >   "Your compiler is not emitting '.note.gnu.property' sections: forcing
> > > > >    support for BTI in the linker, but consider upgrading your toolchain."
> > > > 
> > > > Well, the theory behind the warning is that if the compiler is emitting
> > > > code suitable for the features described in the note then it should
> > > > always emit the appropriate annotations so the warning is more intended
> > > > to be telling the user that the code is trying to link in code that's
> > > > not built properly and will most likely fail at runtime.  In the current
> > > > situation that's an issue with the toolchain not emitting the
> > > > annotations but the common case expectation is that the issue will be
> > > > that there are object files that weren't built appropriately.
> > > 
> > > Hmm, I suppose, although it's a bit belt-and-braces given that we've got
> > > the right options in KBUILD_CFLAGS. What about:
> > > 
> > > 	"Your compiler is not emitting '.note.gnu.property' sections: forcing
> > > 	 support for BTI in the linker, but check your CFLAGS and consider
> > > 	 upgrading your toolchain."
> > > 
> > > I'd usually not be too bothered, but having run into this yesterday and
> > > not understood the problem, I'd like to save somebody else from puzzling
> > > over this if we can!
> > 
> > I really don't think we should fudge this: if the linker doesn't think
> > the inputs are BTI-enabled then the compiler or linker is broken, or
> > there's a bug in the kernel source tree.
> > 
> > The checking done by the toolchain is important -- if we want to
> > suppress it, we should have an override option than depends on BROKEN
> > (because yes, you're explicitly risking a broken kernel if you do this).
> > 
> > The fact that all gcc and clang both screwed this up in various ways at
> > some point is not our fault, or our problem, providing that fixes are
> > available...
> > 
> > Am I being too paranoid?
> 
> I don't think so, but I'm just looking for an answer to "What do we do if
> people start running into this warning?". As it stands, it sounds like it's
> unlikely that they will, but if they do then we're going to have to hack
> something to make it go away.

If we can do it in a way that makes it obvious that we consider it
unsafe, I guess having an override would be OK.  I feel the user should
be required to do something explicit like enable a dangerous-looking
Kconfig option, rather than us helpfully working around it
automatically.

Unless someone can explain why this is not dangerous :)

Cheers
---Dave

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

Re: [PATCH v2 07/10] arm64: asm: Provide a mechanism for generating ELF note for BTI

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1198 bytes --]

On Wed, May 06, 2020 at 02:44:34PM +0100, Will Deacon wrote:
> On Wed, May 06, 2020 at 01:38:55PM +0100, Mark Brown wrote:

> > Right, I was just expecting to have the ifdefs selecting the flags to
> > emit in the middle of the asm macro definiton rather than separately - I
> > didn't see a huge win in defining a macro with the only user being
> > another macro.  I can do something along those lines though.

> With my suggestion, we still only have the 'emit_aarch64_feature_1_and'
> macro, it just provides a way to override the properties if we need that
> later on. All I'm proposing is adding the optional feat parameter, which
> defaults to all of the properties we know about.

> > That will result in us emitting the note with no flags set which
> > *should* be totally fine but is a bit unusual and feels like tempting
> > fate.

> Nah, that's just the dummy .macro definition.

I see - I had been reading the idea as being to have the macro outside
the #ifdef for BTI so that it's usable separately from that and that
you'd just not updated the ifdefs while sketching it out.  I think I've
got a sensible way of achieving that without too much pain though so it
should be fine anyway.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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

Re: [PATCH v2 02/10] arm64: asm: Override SYM_FUNC_START when building the kernel with BTI

[Will Deacon]

On Wed, May 06, 2020 at 04:25:52PM +0100, Mark Brown wrote:
> On Wed, May 06, 2020 at 03:45:44PM +0100, Will Deacon wrote:
> > On Wed, May 06, 2020 at 02:40:21PM +0100, Dave Martin wrote:
> 
> > > I really don't think we should fudge this: if the linker doesn't think
> > > the inputs are BTI-enabled then the compiler or linker is broken, or
> > > there's a bug in the kernel source tree.
> 
> > > The checking done by the toolchain is important -- if we want to
> > > suppress it, we should have an override option than depends on BROKEN
> > > (because yes, you're explicitly risking a broken kernel if you do this).
> 
> > > The fact that all gcc and clang both screwed this up in various ways at
> > > some point is not our fault, or our problem, providing that fixes are
> > > available...
> 
> > > Am I being too paranoid?
> 
> That's my position too, we want the warning - I think if we're going to
> do any handling it should either be to prevent use of kernel BTI
> entirely or to do what Will was originally suggesting and print some
> explanatory text somewhere.  My inclination is that the former is safer
> and fits more with the general architecture approach to this sort of
> thing.
> 
> > I don't think so, but I'm just looking for an answer to "What do we do if
> > people start running into this warning?". As it stands, it sounds like it's
> > unlikely that they will, but if they do then we're going to have to hack
> > something to make it go away.
> 
> It's possible that this e-mail thread showing up in searches might well
> answer people's questions anyway (it is going to be one of the few hits
> for the warning ATM).  How about we deal with this when we get to it, or
> at least as a followup?

Yes, I don't think it's worth writing patches for this now, just wanted
to get a vague idea of our options if people start complaining.

Cheers,

Will

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