[PATCH 1/2] kasan: arm64: set TCR_EL1.TBID1 when enabled

[PATCH 1/2] kasan: arm64: set TCR_EL1.TBID1 when enabled

[Peter Collingbourne]

On hardware supporting pointer authentication, we previously ended up
enabling TBI on instruction accesses when tag-based ASAN was enabled,
but this was costing us 8 bits of PAC entropy, which was unnecessary
since tag-based ASAN does not require TBI on instruction accesses. Get
them back by setting TCR_EL1.TBID1.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/I3dded7824be2e70ea64df0aabab9598d5aebfcc4
---
 arch/arm64/include/asm/pgtable-hwdef.h | 1 +
 arch/arm64/mm/proc.S                   | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index 01a96d07ae74..42442a0ae2ab 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -260,6 +260,7 @@
 #define TCR_TBI1		(UL(1) << 38)
 #define TCR_HA			(UL(1) << 39)
 #define TCR_HD			(UL(1) << 40)
+#define TCR_TBID1		(UL(1) << 52)
 #define TCR_NFD0		(UL(1) << 53)
 #define TCR_NFD1		(UL(1) << 54)
 #define TCR_E0PD0		(UL(1) << 55)
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 23c326a06b2d..97a97a61a8dc 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -40,7 +40,7 @@
 #define TCR_CACHE_FLAGS	TCR_IRGN_WBWA | TCR_ORGN_WBWA
 
 #ifdef CONFIG_KASAN_SW_TAGS
-#define TCR_KASAN_FLAGS TCR_TBI1
+#define TCR_KASAN_FLAGS TCR_TBI1 | TCR_TBID1
 #else
 #define TCR_KASAN_FLAGS 0
 #endif
-- 
2.29.2.454.gaff20da3a2-goog


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

[PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Peter Collingbourne]

Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
set at boot time.

Setting TCR_EL1.TBID0 increases the number of signature bits used by
the pointer authentication instructions for instruction addresses by 8,
which improves the security of pointer authentication, but it also has
the consequence of changing the operation of the branch instructions
so that they no longer ignore the top byte of the target address but
instead fault if they are non-zero. Since this is a change to the
userspace ABI the option defaults to off.

Signed-off-by: Peter Collingbourne <pcc@google.com>
Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
---
This is more of an RFC. An open question is how to expose this.
Having it be a build-time flag is probably the simplest option
but I guess it could also be a boot flag. Since it involves an
ABI change we may also want a prctl() so that userspace can
figure out which mode it is in.

I think we should try to avoid it being a per-task property
so that we don't need to swap yet another system register on
task switch.

This goes on top of my FAR_EL1 series because it involves
a change to how FAR_EL1 is handled on instruction aborts.

 arch/arm64/Kconfig                     | 18 ++++++++++++++++++
 arch/arm64/include/asm/compiler.h      | 18 ++++++++++++------
 arch/arm64/include/asm/pgtable-hwdef.h |  1 +
 arch/arm64/include/asm/pointer_auth.h  |  2 +-
 arch/arm64/kernel/ptrace.c             |  8 +++-----
 arch/arm64/mm/fault.c                  | 14 +++++++++++++-
 arch/arm64/mm/proc.S                   |  8 +++++++-
 7 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1515f6f153a0..6ea17249f33f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1532,6 +1532,24 @@ config ARM64_PTR_AUTH
 	  This feature works with FUNCTION_GRAPH_TRACER option only if
 	  DYNAMIC_FTRACE_WITH_REGS is enabled.
 
+config ARM64_TBI_DATA
+	bool "Restrict top-byte ignore to data accesses"
+	help
+	  Normally, the kernel will enable top-byte ignore for instruction
+	  accesses as well as data accesses. With this configuration option
+	  enabled, on hardware supporting pointer authentication top-byte
+	  ignore will only be enabled for data accesses.
+
+	  The most important consequence of this is that it increases
+	  the number of signature bits used by the pointer authentication
+	  instructions for instruction addresses by 8, which improves the
+	  security of pointer authentication, but it also has the consequence
+	  of changing the operation of the branch instructions so that they
+	  no longer ignore the top byte of the target address but instead
+	  fault if they are non-zero. If your userspace does not depend on
+	  branch instructions ignoring the top byte it is recommended to
+	  select this option.
+
 config CC_HAS_BRANCH_PROT_PAC_RET
 	# GCC 9 or later, clang 8 or later
 	def_bool $(cc-option,-mbranch-protection=pac-ret+leaf)
diff --git a/arch/arm64/include/asm/compiler.h b/arch/arm64/include/asm/compiler.h
index 6fb2e6bcc392..7332fd35bf6f 100644
--- a/arch/arm64/include/asm/compiler.h
+++ b/arch/arm64/include/asm/compiler.h
@@ -12,15 +12,21 @@
  * The EL0/EL1 pointer bits used by a pointer authentication code.
  * This is dependent on TBI0/TBI1 being enabled, or bits 63:56 would also apply.
  */
-#define ptrauth_user_pac_mask()		GENMASK_ULL(54, vabits_actual)
+#ifdef CONFIG_ARM64_TBI_DATA
+#define ptrauth_user_insn_pac_mask()	GENMASK_ULL(63, vabits_actual)
+#else
+#define ptrauth_user_insn_pac_mask()	GENMASK_ULL(54, vabits_actual)
+#endif
+#define ptrauth_user_data_pac_mask()	GENMASK_ULL(54, vabits_actual)
 #define ptrauth_kernel_pac_mask()	GENMASK_ULL(63, vabits_actual)
 
 /* Valid for EL0 TTBR0 and EL1 TTBR1 instruction pointers */
-#define ptrauth_clear_pac(ptr)						\
-	((ptr & BIT_ULL(55)) ? (ptr | ptrauth_kernel_pac_mask()) :	\
-			       (ptr & ~ptrauth_user_pac_mask()))
+#define ptrauth_clear_insn_pac(ptr)                                            \
+	((ptr & BIT_ULL(55)) ? (ptr | ptrauth_kernel_pac_mask()) :             \
+				     (ptr & ~ptrauth_user_insn_pac_mask()))
 
-#define __builtin_return_address(val)					\
-	(void *)(ptrauth_clear_pac((unsigned long)__builtin_return_address(val)))
+#define __builtin_return_address(val)                                          \
+	((void *)(ptrauth_clear_insn_pac(                                      \
+		(unsigned long)__builtin_return_address(val))))
 
 #endif /* __ASM_COMPILER_H */
diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
index 42442a0ae2ab..90e69048442d 100644
--- a/arch/arm64/include/asm/pgtable-hwdef.h
+++ b/arch/arm64/include/asm/pgtable-hwdef.h
@@ -260,6 +260,7 @@
 #define TCR_TBI1		(UL(1) << 38)
 #define TCR_HA			(UL(1) << 39)
 #define TCR_HD			(UL(1) << 40)
+#define TCR_TBID0		(UL(1) << 51)
 #define TCR_TBID1		(UL(1) << 52)
 #define TCR_NFD0		(UL(1) << 53)
 #define TCR_NFD1		(UL(1) << 54)
diff --git a/arch/arm64/include/asm/pointer_auth.h b/arch/arm64/include/asm/pointer_auth.h
index c6b4f0603024..a0022867b8ed 100644
--- a/arch/arm64/include/asm/pointer_auth.h
+++ b/arch/arm64/include/asm/pointer_auth.h
@@ -73,7 +73,7 @@ extern int ptrauth_prctl_reset_keys(struct task_struct *tsk, unsigned long arg);
 
 static inline unsigned long ptrauth_strip_insn_pac(unsigned long ptr)
 {
-	return ptrauth_clear_pac(ptr);
+	return ptrauth_clear_insn_pac(ptr);
 }
 
 #define ptrauth_thread_init_user(tsk)					\
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
index 8ac487c84e37..44afc5c3427e 100644
--- a/arch/arm64/kernel/ptrace.c
+++ b/arch/arm64/kernel/ptrace.c
@@ -893,13 +893,11 @@ static int pac_mask_get(struct task_struct *target,
 {
 	/*
 	 * The PAC bits can differ across data and instruction pointers
-	 * depending on TCR_EL1.TBID*, which we may make use of in future, so
-	 * we expose separate masks.
+	 * depending on TCR_EL1.TBID0, so we expose separate masks.
 	 */
-	unsigned long mask = ptrauth_user_pac_mask();
 	struct user_pac_mask uregs = {
-		.data_mask = mask,
-		.insn_mask = mask,
+		.data_mask = ptrauth_user_data_pac_mask(),
+		.insn_mask = ptrauth_user_insn_pac_mask(),
 	};
 
 	if (!system_supports_address_auth())
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 29a6b8c9e830..617f9f43f528 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -458,11 +458,23 @@ static int __kprobes do_page_fault(unsigned long far, unsigned int esr,
 	vm_fault_t fault;
 	unsigned long vm_flags = VM_ACCESS_FLAGS;
 	unsigned int mm_flags = FAULT_FLAG_DEFAULT;
-	unsigned long addr = untagged_addr(far);
+	unsigned long addr;
 
 	if (kprobe_page_fault(regs, esr))
 		return 0;
 
+	/*
+	 * If TBID0 is set then we may get an IABT with a tagged address here as
+	 * a result of branching to a tagged address. In this case we want to
+	 * avoid untagging the address, let the VMA lookup fail and get a
+	 * SIGSEGV. Leaving the address as is will also work if TBID0 is clear
+	 * or unsupported because the tag bits of FAR_EL1 will be clear.
+	 */
+	if (is_el0_instruction_abort(esr))
+		addr = far;
+	else
+		addr = untagged_addr(far);
+
 	/*
 	 * If we're in an interrupt or have no user context, we must not take
 	 * the fault.
diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 97a97a61a8dc..0e715b9604a1 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -45,6 +45,12 @@
 #define TCR_KASAN_FLAGS 0
 #endif
 
+#ifdef CONFIG_ARM64_TBI_DATA
+#define TCR_TBI_DATA_FLAGS TCR_TBID0
+#else
+#define TCR_TBI_DATA_FLAGS 0
+#endif
+
 /*
  * Default MAIR_EL1. MT_NORMAL_TAGGED is initially mapped as Normal memory and
  * changed during __cpu_setup to Normal Tagged if the system supports MTE.
@@ -456,7 +462,7 @@ SYM_FUNC_START(__cpu_setup)
 	 */
 	mov_q	x10, TCR_TxSZ(VA_BITS) | TCR_CACHE_FLAGS | TCR_SMP_FLAGS | \
 			TCR_TG_FLAGS | TCR_KASLR_FLAGS | TCR_ASID16 | \
-			TCR_TBI0 | TCR_A1 | TCR_KASAN_FLAGS
+			TCR_TBI0 | TCR_TBI_DATA_FLAGS | TCR_A1 | TCR_KASAN_FLAGS
 	tcr_clear_errata_bits x10, x9, x5
 
 #ifdef CONFIG_ARM64_VA_BITS_52
-- 
2.29.2.454.gaff20da3a2-goog


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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Catalin Marinas]

On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> set at boot time.
> 
> Setting TCR_EL1.TBID0 increases the number of signature bits used by
> the pointer authentication instructions for instruction addresses by 8,
> which improves the security of pointer authentication, but it also has
> the consequence of changing the operation of the branch instructions
> so that they no longer ignore the top byte of the target address but
> instead fault if they are non-zero. Since this is a change to the
> userspace ABI the option defaults to off.
> 
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> ---
> This is more of an RFC. An open question is how to expose this.
> Having it be a build-time flag is probably the simplest option
> but I guess it could also be a boot flag. Since it involves an
> ABI change we may also want a prctl() so that userspace can
> figure out which mode it is in.
> 
> I think we should try to avoid it being a per-task property
> so that we don't need to swap yet another system register on
> task switch.

Having it changed per task at run-time is problematic as this bit may be
cached in the TLB, so it would require a synchronisation across all CPUs
followed by TLBI. It's not even clear to me from the ARM ARM whether
this bit is tagged by ASID, which, if not, would make a per-process
setting impossible.

So this leaves us with a cmdline option. If we are confident that no
software makes use of tagged instruction pointers, we could have it
default on.

Adding Szabolcs on the gcc/glibc side.

-- 
Catalin

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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Peter Collingbourne]

On Tue, Nov 24, 2020 at 10:47 AM Catalin Marinas
<catalin.marinas@arm.com> wrote:
>
> On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> > Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> > set at boot time.
> >
> > Setting TCR_EL1.TBID0 increases the number of signature bits used by
> > the pointer authentication instructions for instruction addresses by 8,
> > which improves the security of pointer authentication, but it also has
> > the consequence of changing the operation of the branch instructions
> > so that they no longer ignore the top byte of the target address but
> > instead fault if they are non-zero. Since this is a change to the
> > userspace ABI the option defaults to off.
> >
> > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> > ---
> > This is more of an RFC. An open question is how to expose this.
> > Having it be a build-time flag is probably the simplest option
> > but I guess it could also be a boot flag. Since it involves an
> > ABI change we may also want a prctl() so that userspace can
> > figure out which mode it is in.
> >
> > I think we should try to avoid it being a per-task property
> > so that we don't need to swap yet another system register on
> > task switch.
>
> Having it changed per task at run-time is problematic as this bit may be
> cached in the TLB, so it would require a synchronisation across all CPUs
> followed by TLBI. It's not even clear to me from the ARM ARM whether
> this bit is tagged by ASID, which, if not, would make a per-process
> setting impossible.
>
> So this leaves us with a cmdline option. If we are confident that no
> software makes use of tagged instruction pointers, we could have it
> default on.

I would be concerned about turning it on by default because tagged
instruction pointers may end up being used unintentionally as a result
of emergent behavior. For example, when booting Android under FVP with
this enabled I discovered that SwiftShader would crash when entering
JITed code because the code was being stored at a tagged address
(tagged because it had been allocated using Scudo's MTE allocator).
Arguably software shouldn't be storing executable code in memory owned
by the allocator as this would require changing the permissions of
memory that it doesn't own, but from the kernel's perspective it is
valid.

Peter

> Adding Szabolcs on the gcc/glibc side.
>
> --
> Catalin

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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Szabolcs Nagy]

The 11/24/2020 11:18, Peter Collingbourne wrote:
> On Tue, Nov 24, 2020 at 10:47 AM Catalin Marinas
> <catalin.marinas@arm.com> wrote:
> > On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> > > Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> > > set at boot time.
> > >
> > > Setting TCR_EL1.TBID0 increases the number of signature bits used by
> > > the pointer authentication instructions for instruction addresses by 8,
> > > which improves the security of pointer authentication, but it also has
> > > the consequence of changing the operation of the branch instructions
> > > so that they no longer ignore the top byte of the target address but
> > > instead fault if they are non-zero. Since this is a change to the
> > > userspace ABI the option defaults to off.
> > >
> > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> > > ---
> > > This is more of an RFC. An open question is how to expose this.
> > > Having it be a build-time flag is probably the simplest option
> > > but I guess it could also be a boot flag. Since it involves an
> > > ABI change we may also want a prctl() so that userspace can
> > > figure out which mode it is in.
> > >
> > > I think we should try to avoid it being a per-task property
> > > so that we don't need to swap yet another system register on
> > > task switch.
> >
> > Having it changed per task at run-time is problematic as this bit may be
> > cached in the TLB, so it would require a synchronisation across all CPUs
> > followed by TLBI. It's not even clear to me from the ARM ARM whether
> > this bit is tagged by ASID, which, if not, would make a per-process
> > setting impossible.
> >
> > So this leaves us with a cmdline option. If we are confident that no
> > software makes use of tagged instruction pointers, we could have it
> > default on.
> 
> I would be concerned about turning it on by default because tagged
> instruction pointers may end up being used unintentionally as a result
> of emergent behavior. For example, when booting Android under FVP with
> this enabled I discovered that SwiftShader would crash when entering
> JITed code because the code was being stored at a tagged address
> (tagged because it had been allocated using Scudo's MTE allocator).
> Arguably software shouldn't be storing executable code in memory owned
> by the allocator as this would require changing the permissions of
> memory that it doesn't own, but from the kernel's perspective it is
> valid.

it might be still possible to change this abi on linux by
default, but i don't know what's the right way to manage the
abi transition. i will have to think about it.

i dont think PROT_MTE|PROT_EXEC is architecturally well
supported (e.g. to have different colored functions or
similar, pc relative addressing doesn't work right).

(tbi for instruction pointers is unlikely to be useful, but
extra 8 bits for pac is useful. i think we should be able to
move to an abi that is compatible with either setting.)

(i think supporting mmap/munmap/madvise/mprotect on malloced
memory is problematic in general not just with heap tagging
so it would be nice to fix whatever jit that marks malloced
memory as executable.)

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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Peter Collingbourne]

On Wed, Nov 25, 2020 at 6:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 11/24/2020 11:18, Peter Collingbourne wrote:
> > On Tue, Nov 24, 2020 at 10:47 AM Catalin Marinas
> > <catalin.marinas@arm.com> wrote:
> > > On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> > > > Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> > > > set at boot time.
> > > >
> > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by
> > > > the pointer authentication instructions for instruction addresses by 8,
> > > > which improves the security of pointer authentication, but it also has
> > > > the consequence of changing the operation of the branch instructions
> > > > so that they no longer ignore the top byte of the target address but
> > > > instead fault if they are non-zero. Since this is a change to the
> > > > userspace ABI the option defaults to off.
> > > >
> > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> > > > ---
> > > > This is more of an RFC. An open question is how to expose this.
> > > > Having it be a build-time flag is probably the simplest option
> > > > but I guess it could also be a boot flag. Since it involves an
> > > > ABI change we may also want a prctl() so that userspace can
> > > > figure out which mode it is in.
> > > >
> > > > I think we should try to avoid it being a per-task property
> > > > so that we don't need to swap yet another system register on
> > > > task switch.
> > >
> > > Having it changed per task at run-time is problematic as this bit may be
> > > cached in the TLB, so it would require a synchronisation across all CPUs
> > > followed by TLBI. It's not even clear to me from the ARM ARM whether
> > > this bit is tagged by ASID, which, if not, would make a per-process
> > > setting impossible.
> > >
> > > So this leaves us with a cmdline option. If we are confident that no
> > > software makes use of tagged instruction pointers, we could have it
> > > default on.
> >
> > I would be concerned about turning it on by default because tagged
> > instruction pointers may end up being used unintentionally as a result
> > of emergent behavior. For example, when booting Android under FVP with
> > this enabled I discovered that SwiftShader would crash when entering
> > JITed code because the code was being stored at a tagged address
> > (tagged because it had been allocated using Scudo's MTE allocator).
> > Arguably software shouldn't be storing executable code in memory owned
> > by the allocator as this would require changing the permissions of
> > memory that it doesn't own, but from the kernel's perspective it is
> > valid.
>
> it might be still possible to change this abi on linux by
> default, but i don't know what's the right way to manage the
> abi transition. i will have to think about it.
>
> i dont think PROT_MTE|PROT_EXEC is architecturally well
> supported (e.g. to have different colored functions or
> similar, pc relative addressing doesn't work right).
>
> (tbi for instruction pointers is unlikely to be useful, but
> extra 8 bits for pac is useful. i think we should be able to
> move to an abi that is compatible with either setting.)
>
> (i think supporting mmap/munmap/madvise/mprotect on malloced
> memory is problematic in general not just with heap tagging
> so it would be nice to fix whatever jit that marks malloced
> memory as executable.)

Hi Szabolcs,

Did you get a chance to think about this?

I propose that we start with a command line option that defaults to
off. If/when any ABI transition happens we can change the default.

Peter

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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Szabolcs Nagy]

The 06/15/2021 16:41, Peter Collingbourne wrote:
> On Wed, Nov 25, 2020 at 6:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> >
> > The 11/24/2020 11:18, Peter Collingbourne wrote:
> > > On Tue, Nov 24, 2020 at 10:47 AM Catalin Marinas
> > > <catalin.marinas@arm.com> wrote:
> > > > On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> > > > > Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> > > > > set at boot time.
> > > > >
> > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by
> > > > > the pointer authentication instructions for instruction addresses by 8,
> > > > > which improves the security of pointer authentication, but it also has
> > > > > the consequence of changing the operation of the branch instructions
> > > > > so that they no longer ignore the top byte of the target address but
> > > > > instead fault if they are non-zero. Since this is a change to the
> > > > > userspace ABI the option defaults to off.
> > > > >
> > > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > > Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> > > > > ---
> > > > > This is more of an RFC. An open question is how to expose this.
> > > > > Having it be a build-time flag is probably the simplest option
> > > > > but I guess it could also be a boot flag. Since it involves an
> > > > > ABI change we may also want a prctl() so that userspace can
> > > > > figure out which mode it is in.
> > > > >
> > > > > I think we should try to avoid it being a per-task property
> > > > > so that we don't need to swap yet another system register on
> > > > > task switch.
> > > >
> > > > Having it changed per task at run-time is problematic as this bit may be
> > > > cached in the TLB, so it would require a synchronisation across all CPUs
> > > > followed by TLBI. It's not even clear to me from the ARM ARM whether
> > > > this bit is tagged by ASID, which, if not, would make a per-process
> > > > setting impossible.
> > > >
> > > > So this leaves us with a cmdline option. If we are confident that no
> > > > software makes use of tagged instruction pointers, we could have it
> > > > default on.
> > >
> > > I would be concerned about turning it on by default because tagged
> > > instruction pointers may end up being used unintentionally as a result
> > > of emergent behavior. For example, when booting Android under FVP with
> > > this enabled I discovered that SwiftShader would crash when entering
> > > JITed code because the code was being stored at a tagged address
> > > (tagged because it had been allocated using Scudo's MTE allocator).
> > > Arguably software shouldn't be storing executable code in memory owned
> > > by the allocator as this would require changing the permissions of
> > > memory that it doesn't own, but from the kernel's perspective it is
> > > valid.
> >
> > it might be still possible to change this abi on linux by
> > default, but i don't know what's the right way to manage the
> > abi transition. i will have to think about it.
> >
> > i dont think PROT_MTE|PROT_EXEC is architecturally well
> > supported (e.g. to have different colored functions or
> > similar, pc relative addressing doesn't work right).
> >
> > (tbi for instruction pointers is unlikely to be useful, but
> > extra 8 bits for pac is useful. i think we should be able to
> > move to an abi that is compatible with either setting.)
> >
> > (i think supporting mmap/munmap/madvise/mprotect on malloced
> > memory is problematic in general not just with heap tagging
> > so it would be nice to fix whatever jit that marks malloced
> > memory as executable.)
> 
> Hi Szabolcs,
> 
> Did you get a chance to think about this?
> 
> I propose that we start with a command line option that defaults to
> off. If/when any ABI transition happens we can change the default.

a default off per boot option sounds safe even if
there are some incompatible binaries.

(i assume virtualization works: host and guest can
have different settings, so users can always run
old systems in kvm.)

however it would be nice to make this part of the
linux platform abi and avoid fragmentation.
the difficult bits are

- unclear trade-off: does the abi change have
  adverse effect on potential tbi/mte/.. use?
  or even existing usage? (data only tbi is a
  more complicated concept than plain tbi)

- incompatibility cannot be statically detected.
  (toolchain cannot diagnose or enforce it,
  it is only detectable at runtime, on a system
  with the changed setting.)

i think in c the usability of tbi is very limited
now. the libc and compiler needs a tbi abi for it
to work (e.g. different tags on pointers to the
same object break pointer comparision). if tagged
pointers are not dereferenced and don't escape
then tbi is not needed. so only custom runtimes
and implementation internal tooling like hwasan
or heap tagging can use it. arbitrary user tagged
pointer is not supported in libc now.

there is glibc discussion about the libc tbi abi
because intel LAM needs it too, i don't know yet
how that will work, but we can try to make it
compatible with the new setting and roll that
out as the right way to use tbi.


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

Re: [PATCH 2/2] arm64: allow TCR_EL1.TBID0 to be configured

[Peter Collingbourne]

On Wed, Jun 16, 2021 at 5:56 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
>
> The 06/15/2021 16:41, Peter Collingbourne wrote:
> > On Wed, Nov 25, 2020 at 6:38 AM Szabolcs Nagy <szabolcs.nagy@arm.com> wrote:
> > >
> > > The 11/24/2020 11:18, Peter Collingbourne wrote:
> > > > On Tue, Nov 24, 2020 at 10:47 AM Catalin Marinas
> > > > <catalin.marinas@arm.com> wrote:
> > > > > On Sat, Nov 21, 2020 at 01:59:03AM -0800, Peter Collingbourne wrote:
> > > > > > Introduce a Kconfig option that controls whether TCR_EL1.TBID0 is
> > > > > > set at boot time.
> > > > > >
> > > > > > Setting TCR_EL1.TBID0 increases the number of signature bits used by
> > > > > > the pointer authentication instructions for instruction addresses by 8,
> > > > > > which improves the security of pointer authentication, but it also has
> > > > > > the consequence of changing the operation of the branch instructions
> > > > > > so that they no longer ignore the top byte of the target address but
> > > > > > instead fault if they are non-zero. Since this is a change to the
> > > > > > userspace ABI the option defaults to off.
> > > > > >
> > > > > > Signed-off-by: Peter Collingbourne <pcc@google.com>
> > > > > > Link: https://linux-review.googlesource.com/id/Ife724ad708142bc475f42e8c1d9609124994bbbd
> > > > > > ---
> > > > > > This is more of an RFC. An open question is how to expose this.
> > > > > > Having it be a build-time flag is probably the simplest option
> > > > > > but I guess it could also be a boot flag. Since it involves an
> > > > > > ABI change we may also want a prctl() so that userspace can
> > > > > > figure out which mode it is in.
> > > > > >
> > > > > > I think we should try to avoid it being a per-task property
> > > > > > so that we don't need to swap yet another system register on
> > > > > > task switch.
> > > > >
> > > > > Having it changed per task at run-time is problematic as this bit may be
> > > > > cached in the TLB, so it would require a synchronisation across all CPUs
> > > > > followed by TLBI. It's not even clear to me from the ARM ARM whether
> > > > > this bit is tagged by ASID, which, if not, would make a per-process
> > > > > setting impossible.
> > > > >
> > > > > So this leaves us with a cmdline option. If we are confident that no
> > > > > software makes use of tagged instruction pointers, we could have it
> > > > > default on.
> > > >
> > > > I would be concerned about turning it on by default because tagged
> > > > instruction pointers may end up being used unintentionally as a result
> > > > of emergent behavior. For example, when booting Android under FVP with
> > > > this enabled I discovered that SwiftShader would crash when entering
> > > > JITed code because the code was being stored at a tagged address
> > > > (tagged because it had been allocated using Scudo's MTE allocator).
> > > > Arguably software shouldn't be storing executable code in memory owned
> > > > by the allocator as this would require changing the permissions of
> > > > memory that it doesn't own, but from the kernel's perspective it is
> > > > valid.
> > >
> > > it might be still possible to change this abi on linux by
> > > default, but i don't know what's the right way to manage the
> > > abi transition. i will have to think about it.
> > >
> > > i dont think PROT_MTE|PROT_EXEC is architecturally well
> > > supported (e.g. to have different colored functions or
> > > similar, pc relative addressing doesn't work right).
> > >
> > > (tbi for instruction pointers is unlikely to be useful, but
> > > extra 8 bits for pac is useful. i think we should be able to
> > > move to an abi that is compatible with either setting.)
> > >
> > > (i think supporting mmap/munmap/madvise/mprotect on malloced
> > > memory is problematic in general not just with heap tagging
> > > so it would be nice to fix whatever jit that marks malloced
> > > memory as executable.)
> >
> > Hi Szabolcs,
> >
> > Did you get a chance to think about this?
> >
> > I propose that we start with a command line option that defaults to
> > off. If/when any ABI transition happens we can change the default.
>
> a default off per boot option sounds safe even if
> there are some incompatible binaries.

Okay, let's start with that then, although I'm about 75% sure we
should be able to change the default if we push for it. I've sent a v2
with that implemented.

> (i assume virtualization works: host and guest can
> have different settings, so users can always run
> old systems in kvm.)
>
> however it would be nice to make this part of the
> linux platform abi and avoid fragmentation.
> the difficult bits are
>
> - unclear trade-off: does the abi change have
>   adverse effect on potential tbi/mte/.. use?
>   or even existing usage? (data only tbi is a
>   more complicated concept than plain tbi)

I think it's unlikely that there will be such an effect, at least for
existing usage. Note that Apple has been shipping kernels that enable
TBID0 for several years now and I'm not aware of any consequent
breakage as a result (and as far as I know Apple is a more heavy user
of TBI than anything in the Linux world, sanitizers excepted, given
that e.g. the Swift implementation relies on it).

> - incompatibility cannot be statically detected.
>   (toolchain cannot diagnose or enforce it,
>   it is only detectable at runtime, on a system
>   with the changed setting.)

That is unfortunate, although it later occurred to me that in most
cases the incompatibility should manifest in a particular form, where
the program sets a tagged PC and we enter an instruction abort. This
puts us in the kernel, which may choose to handle the abort by
clearing the tag and resuming the program. At that point, the TBID
feature is basically unobservable for programs that do not use PAC
instructions.

This would of course slow down programs that rely on tagged
instruction pointers, and basically destroys the security of PAC for
instruction pointers by making the hardware error code ineffective (at
least on machines that do not implement enhanced PAC2), so I would
view this as purely an opt-in compatibility scheme to keep old
programs (which by virtue of being old wouldn't be using PAC
instructions) working (albeit more slowly and less securely) rather
than something that new programs should use.

I'm not sure if we should implement this scheme straight away. But I
think the more important thing is that it can be done, which means
that I think it should be relatively safe to turn on the ABI
system-wide and rely on such a scheme to keep old programs working (if
any).

> i think in c the usability of tbi is very limited
> now. the libc and compiler needs a tbi abi for it
> to work (e.g. different tags on pointers to the
> same object break pointer comparision). if tagged
> pointers are not dereferenced and don't escape
> then tbi is not needed. so only custom runtimes
> and implementation internal tooling like hwasan
> or heap tagging can use it. arbitrary user tagged
> pointer is not supported in libc now.

Yes, I think that's a good reason why this is unlikely to break anything.

Peter

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

Re: [PATCH 1/2] kasan: arm64: set TCR_EL1.TBID1 when enabled

[Andrey Konovalov]

On Sat, Nov 21, 2020 at 10:59 AM Peter Collingbourne <pcc@google.com> wrote:
>
> On hardware supporting pointer authentication, we previously ended up
> enabling TBI on instruction accesses when tag-based ASAN was enabled,
> but this was costing us 8 bits of PAC entropy, which was unnecessary
> since tag-based ASAN does not require TBI on instruction accesses. Get
> them back by setting TCR_EL1.TBID1.
>
> Signed-off-by: Peter Collingbourne <pcc@google.com>
> Link: https://linux-review.googlesource.com/id/I3dded7824be2e70ea64df0aabab9598d5aebfcc4
> ---
>  arch/arm64/include/asm/pgtable-hwdef.h | 1 +
>  arch/arm64/mm/proc.S                   | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/pgtable-hwdef.h b/arch/arm64/include/asm/pgtable-hwdef.h
> index 01a96d07ae74..42442a0ae2ab 100644
> --- a/arch/arm64/include/asm/pgtable-hwdef.h
> +++ b/arch/arm64/include/asm/pgtable-hwdef.h
> @@ -260,6 +260,7 @@
>  #define TCR_TBI1               (UL(1) << 38)
>  #define TCR_HA                 (UL(1) << 39)
>  #define TCR_HD                 (UL(1) << 40)
> +#define TCR_TBID1              (UL(1) << 52)
>  #define TCR_NFD0               (UL(1) << 53)
>  #define TCR_NFD1               (UL(1) << 54)
>  #define TCR_E0PD0              (UL(1) << 55)
> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
> index 23c326a06b2d..97a97a61a8dc 100644
> --- a/arch/arm64/mm/proc.S
> +++ b/arch/arm64/mm/proc.S
> @@ -40,7 +40,7 @@
>  #define TCR_CACHE_FLAGS        TCR_IRGN_WBWA | TCR_ORGN_WBWA
>
>  #ifdef CONFIG_KASAN_SW_TAGS
> -#define TCR_KASAN_FLAGS TCR_TBI1
> +#define TCR_KASAN_FLAGS TCR_TBI1 | TCR_TBID1
>  #else
>  #define TCR_KASAN_FLAGS 0
>  #endif
> --
> 2.29.2.454.gaff20da3a2-goog
>

Reviewed-by: Andrey Konovalov <andreyknvl@google.com>

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

Re: [PATCH 1/2] kasan: arm64: set TCR_EL1.TBID1 when enabled

[Catalin Marinas]

On Sat, 21 Nov 2020 01:59:02 -0800, Peter Collingbourne wrote:
> On hardware supporting pointer authentication, we previously ended up
> enabling TBI on instruction accesses when tag-based ASAN was enabled,
> but this was costing us 8 bits of PAC entropy, which was unnecessary
> since tag-based ASAN does not require TBI on instruction accesses. Get
> them back by setting TCR_EL1.TBID1.

Applied to arm64 (for-next/misc), thanks!

[1/2] kasan: arm64: set TCR_EL1.TBID1 when enabled
      https://git.kernel.org/arm64/c/49b3cf035edc

This patch looks safe. Patch 2/2 needs more discussions.

-- 
Catalin


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