[PATCH v2 0/4] arm64: improve tagged pointer handling

[PATCH v2 0/4] arm64: improve tagged pointer handling

[Kristina Martsenko]

Hi,

Here are some patches to fix a few issues related to tagged pointer
handling.

Tagged pointers from userspace can end up in the kernel in a number of
ways. I most likely have not found all of them, but they include at
least the following:

 - Passing tagged pointers in system call arguments. This would be a
   userspace bug, as documented in tagged-pointers.txt.

 - Through FAR_EL1 when we take a data abort or watchpoint exception.
   Watchpoint handling is currently broken if we get a tagged pointer,
   patch #2 in this series fixes it. We already do the right thing for
   data aborts but patch #3 tries to improve on it a little.

 - Reading a tagged pointer from a GPR when trapping and emulating
   instructions, e.g. cache maintenance or uprobes. Patch #1 fixes the
   cache maintenance case.

 - The user stack pointer, frame pointer (x29), frame records, and link
   register (x30) can contain tagged pointers. Patch #4 documents that
   some kernel features do not currently work with tagged pointers in
   the first three of these.

 - A tagged pointer can end up in the PC on an illegal exception return
   (see D4.1.4 ARMARM A.k_iss10775), and from there in ELR on exception
   entry. As I understand it, this can only be caused by a bad eret at
   EL1 or a bad debug state exit by an external debugger, so only by a
   bug in Linux/firmware or the external debugger. So I don't think we
   need to handle this.

Note that the above applies to Linux only. I have spoken to Marc Zyngier
about KVM, and so far he hasn't found any problems there.

Thanks,
Kristina

v2:
 - Patch #3: changed clear_address_tag macro arguments, swapped bic and
   tst


Kristina Martsenko (4):
  arm64: traps: fix userspace cache maintenance emulation on a tagged
    pointer
  arm64: hw_breakpoint: fix watchpoint matching for tagged pointers
  arm64: entry: improve data abort handling of tagged pointers
  arm64: documentation: document tagged pointer stack constraints

 Documentation/arm64/tagged-pointers.txt | 62 +++++++++++++++++++++++++--------
 arch/arm64/include/asm/asm-uaccess.h    |  9 +++++
 arch/arm64/include/asm/uaccess.h        |  6 ++--
 arch/arm64/kernel/entry.S               |  5 +--
 arch/arm64/kernel/hw_breakpoint.c       |  3 ++
 arch/arm64/kernel/traps.c               |  4 +--
 6 files changed, 67 insertions(+), 22 deletions(-)

-- 
2.1.4

[PATCH v2 1/4] arm64: traps: fix userspace cache maintenance emulation on a tagged pointer

[Kristina Martsenko]

When we emulate userspace cache maintenance in the kernel, we can
currently send the task a SIGSEGV even though the maintenance was done
on a valid address. This happens if the address has a non-zero address
tag, and happens to not be mapped in.

When we get the address from a user register, we don't currently remove
the address tag before performing cache maintenance on it. If the
maintenance faults, we end up in either __do_page_fault, where find_vma
can't find the VMA if the address has a tag, or in do_translation_fault,
where the tagged address will appear to be above TASK_SIZE. In both
cases, the address is not mapped in, and the task is sent a SIGSEGV.

This patch removes the tag from the address before using it. With this
patch, the fault is handled correctly, the address gets mapped in, and
the cache maintenance succeeds.

As a second bug, if cache maintenance (correctly) fails on an invalid
tagged address, the address gets passed into arm64_notify_segfault,
where find_vma fails to find the VMA due to the tag, and the wrong
si_code may be sent as part of the siginfo_t of the segfault. With this
patch, the correct si_code is sent.

Fixes: 7dd01aef0557 ("arm64: trap userspace "dc cvau" cache operation on errata-affected core")
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
---

Note that patch #3 would also fix the first bug (incorrect segfault),
but not the second (wrong si_code), hence this patch.

 arch/arm64/kernel/traps.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index e52be6aa44ee..45c8eca951bc 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -443,7 +443,7 @@ int cpu_enable_cache_maint_trap(void *__unused)
 }
 
 #define __user_cache_maint(insn, address, res)			\
-	if (untagged_addr(address) >= user_addr_max()) {	\
+	if (address >= user_addr_max()) {			\
 		res = -EFAULT;					\
 	} else {						\
 		uaccess_ttbr0_enable();				\
@@ -469,7 +469,7 @@ static void user_cache_maint_handler(unsigned int esr, struct pt_regs *regs)
 	int crm = (esr & ESR_ELx_SYS64_ISS_CRM_MASK) >> ESR_ELx_SYS64_ISS_CRM_SHIFT;
 	int ret = 0;
 
-	address = pt_regs_read_reg(regs, rt);
+	address = untagged_addr(pt_regs_read_reg(regs, rt));
 
 	switch (crm) {
 	case ESR_ELx_SYS64_ISS_CRM_DC_CVAU:	/* DC CVAU, gets promoted */
-- 
2.1.4

[PATCH v2 2/4] arm64: hw_breakpoint: fix watchpoint matching for tagged pointers

[Kristina Martsenko]

When we take a watchpoint exception, the address that triggered the
watchpoint is found in FAR_EL1. We compare it to the address of each
configured watchpoint to see which one was hit.

The configured watchpoint addresses are untagged, while the address in
FAR_EL1 will have an address tag if the data access was done using a
tagged address. The tag needs to be removed to compare the address to
the watchpoints.

Currently we don't remove it, and as a result can report the wrong
watchpoint as being hit (specifically, always either the highest TTBR0
watchpoint or lowest TTBR1 watchpoint). This patch removes the tag.

Fixes: d50240a5f6ce ("arm64: mm: permit use of tagged pointers at EL0")
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
---
 arch/arm64/include/asm/uaccess.h  | 6 +++---
 arch/arm64/kernel/hw_breakpoint.c | 3 +++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index 5308d696311b..0221029e27ff 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -106,9 +106,9 @@ static inline void set_fs(mm_segment_t fs)
 })
 
 /*
- * When dealing with data aborts or instruction traps we may end up with
- * a tagged userland pointer. Clear the tag to get a sane pointer to pass
- * on to access_ok(), for instance.
+ * When dealing with data aborts, watchpoints, or instruction traps we may end
+ * up with a tagged userland pointer. Clear the tag to get a sane pointer to
+ * pass on to access_ok(), for instance.
  */
 #define untagged_addr(addr)		sign_extend64(addr, 55)
 
diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
index 0296e7924240..749f81779420 100644
--- a/arch/arm64/kernel/hw_breakpoint.c
+++ b/arch/arm64/kernel/hw_breakpoint.c
@@ -36,6 +36,7 @@
 #include <asm/traps.h>
 #include <asm/cputype.h>
 #include <asm/system_misc.h>
+#include <asm/uaccess.h>
 
 /* Breakpoint currently in use for each BRP. */
 static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
@@ -721,6 +722,8 @@ static u64 get_distance_from_watchpoint(unsigned long addr, u64 val,
 	u64 wp_low, wp_high;
 	u32 lens, lene;
 
+	addr = untagged_addr(addr);
+
 	lens = __ffs(ctrl->len);
 	lene = __fls(ctrl->len);
 
-- 
2.1.4

[PATCH v2 2/4] arm64: hw_breakpoint: fix watchpoint matching for tagged pointers

[Mark Rutland]

On Wed, May 03, 2017 at 04:37:46PM +0100, Kristina Martsenko wrote:
> When we take a watchpoint exception, the address that triggered the
> watchpoint is found in FAR_EL1. We compare it to the address of each
> configured watchpoint to see which one was hit.
> 
> The configured watchpoint addresses are untagged, while the address in
> FAR_EL1 will have an address tag if the data access was done using a
> tagged address. The tag needs to be removed to compare the address to
> the watchpoints.
> 
> Currently we don't remove it, and as a result can report the wrong
> watchpoint as being hit (specifically, always either the highest TTBR0
> watchpoint or lowest TTBR1 watchpoint). This patch removes the tag.
> 
> Fixes: d50240a5f6ce ("arm64: mm: permit use of tagged pointers at EL0")
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>

Looks sane to me. FWIW:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> ---
>  arch/arm64/include/asm/uaccess.h  | 6 +++---
>  arch/arm64/kernel/hw_breakpoint.c | 3 +++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 5308d696311b..0221029e27ff 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -106,9 +106,9 @@ static inline void set_fs(mm_segment_t fs)
>  })
>  
>  /*
> - * When dealing with data aborts or instruction traps we may end up with
> - * a tagged userland pointer. Clear the tag to get a sane pointer to pass
> - * on to access_ok(), for instance.
> + * When dealing with data aborts, watchpoints, or instruction traps we may end
> + * up with a tagged userland pointer. Clear the tag to get a sane pointer to
> + * pass on to access_ok(), for instance.
>   */
>  #define untagged_addr(addr)		sign_extend64(addr, 55)
>  
> diff --git a/arch/arm64/kernel/hw_breakpoint.c b/arch/arm64/kernel/hw_breakpoint.c
> index 0296e7924240..749f81779420 100644
> --- a/arch/arm64/kernel/hw_breakpoint.c
> +++ b/arch/arm64/kernel/hw_breakpoint.c
> @@ -36,6 +36,7 @@
>  #include <asm/traps.h>
>  #include <asm/cputype.h>
>  #include <asm/system_misc.h>
> +#include <asm/uaccess.h>
>  
>  /* Breakpoint currently in use for each BRP. */
>  static DEFINE_PER_CPU(struct perf_event *, bp_on_reg[ARM_MAX_BRP]);
> @@ -721,6 +722,8 @@ static u64 get_distance_from_watchpoint(unsigned long addr, u64 val,
>  	u64 wp_low, wp_high;
>  	u32 lens, lene;
>  
> +	addr = untagged_addr(addr);
> +
>  	lens = __ffs(ctrl->len);
>  	lene = __fls(ctrl->len);
>  
> -- 
> 2.1.4
> 

[PATCH v2 3/4] arm64: entry: improve data abort handling of tagged pointers

[Kristina Martsenko]

When handling a data abort from EL0, we currently zero the top byte of
the faulting address, as we assume the address is a TTBR0 address, which
may contain a non-zero address tag. However, the address may be a TTBR1
address, in which case we should not zero the top byte. This patch fixes
that. The effect is that the full TTBR1 address is passed to the task's
signal handler (or printed out in the kernel log).

When handling a data abort from EL1, we leave the faulting address
intact, as we assume it's either a TTBR1 address or a TTBR0 address with
tag 0x00. This is true as far as I'm aware, we don't seem to access a
tagged TTBR0 address anywhere in the kernel. Regardless, it's easy to
forget about address tags, and code added in the future may not always
remember to remove tags from addresses before accessing them. So add tag
handling to the EL1 data abort handler as well. This also makes it
consistent with the EL0 data abort handler.

Fixes: d50240a5f6ce ("arm64: mm: permit use of tagged pointers at EL0")
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
---
 arch/arm64/include/asm/asm-uaccess.h | 9 +++++++++
 arch/arm64/kernel/entry.S            | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
index df411f3e083c..ecd9788cd298 100644
--- a/arch/arm64/include/asm/asm-uaccess.h
+++ b/arch/arm64/include/asm/asm-uaccess.h
@@ -62,4 +62,13 @@ alternative_if ARM64_ALT_PAN_NOT_UAO
 alternative_else_nop_endif
 	.endm
 
+/*
+ * Remove the address tag from a virtual address, if present.
+ */
+	.macro	clear_address_tag, dst, addr
+	tst	\addr, #(1 << 55)
+	bic	\dst, \addr, #(0xff << 56)
+	csel	\dst, \dst, \addr, eq
+	.endm
+
 #endif
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 43512d4d7df2..b738880350f9 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -428,12 +428,13 @@ el1_da:
 	/*
 	 * Data abort handling
 	 */
-	mrs	x0, far_el1
+	mrs	x3, far_el1
 	enable_dbg
 	// re-enable interrupts if they were enabled in the aborted context
 	tbnz	x23, #7, 1f			// PSR_I_BIT
 	enable_irq
 1:
+	clear_address_tag x0, x3
 	mov	x2, sp				// struct pt_regs
 	bl	do_mem_abort
 
@@ -594,7 +595,7 @@ el0_da:
 	// enable interrupts before calling the main handler
 	enable_dbg_and_irq
 	ct_user_exit
-	bic	x0, x26, #(0xff << 56)
+	clear_address_tag x0, x26
 	mov	x1, x25
 	mov	x2, sp
 	bl	do_mem_abort
-- 
2.1.4

[PATCH v2 3/4] arm64: entry: improve data abort handling of tagged pointers

[Dave Martin]

On Wed, May 03, 2017 at 04:37:47PM +0100, Kristina Martsenko wrote:
> When handling a data abort from EL0, we currently zero the top byte of
> the faulting address, as we assume the address is a TTBR0 address, which
> may contain a non-zero address tag. However, the address may be a TTBR1
> address, in which case we should not zero the top byte. This patch fixes
> that. The effect is that the full TTBR1 address is passed to the task's
> signal handler (or printed out in the kernel log).
> 
> When handling a data abort from EL1, we leave the faulting address
> intact, as we assume it's either a TTBR1 address or a TTBR0 address with
> tag 0x00. This is true as far as I'm aware, we don't seem to access a
> tagged TTBR0 address anywhere in the kernel. Regardless, it's easy to
> forget about address tags, and code added in the future may not always
> remember to remove tags from addresses before accessing them. So add tag
> handling to the EL1 data abort handler as well. This also makes it
> consistent with the EL0 data abort handler.

FWIW,
Reviewed-by: Dave Martin <Dave.Martin@arm.com>

(Looks like Will beat me to it, though.)

Cheers
---Dave

> 
> Fixes: d50240a5f6ce ("arm64: mm: permit use of tagged pointers at EL0")
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
> ---
>  arch/arm64/include/asm/asm-uaccess.h | 9 +++++++++
>  arch/arm64/kernel/entry.S            | 5 +++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
> index df411f3e083c..ecd9788cd298 100644
> --- a/arch/arm64/include/asm/asm-uaccess.h
> +++ b/arch/arm64/include/asm/asm-uaccess.h
> @@ -62,4 +62,13 @@ alternative_if ARM64_ALT_PAN_NOT_UAO
>  alternative_else_nop_endif
>  	.endm
>  
> +/*
> + * Remove the address tag from a virtual address, if present.
> + */
> +	.macro	clear_address_tag, dst, addr
> +	tst	\addr, #(1 << 55)
> +	bic	\dst, \addr, #(0xff << 56)
> +	csel	\dst, \dst, \addr, eq
> +	.endm
> +
>  #endif
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 43512d4d7df2..b738880350f9 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -428,12 +428,13 @@ el1_da:
>  	/*
>  	 * Data abort handling
>  	 */
> -	mrs	x0, far_el1
> +	mrs	x3, far_el1
>  	enable_dbg
>  	// re-enable interrupts if they were enabled in the aborted context
>  	tbnz	x23, #7, 1f			// PSR_I_BIT
>  	enable_irq
>  1:
> +	clear_address_tag x0, x3
>  	mov	x2, sp				// struct pt_regs
>  	bl	do_mem_abort
>  
> @@ -594,7 +595,7 @@ el0_da:
>  	// enable interrupts before calling the main handler
>  	enable_dbg_and_irq
>  	ct_user_exit
> -	bic	x0, x26, #(0xff << 56)
> +	clear_address_tag x0, x26
>  	mov	x1, x25
>  	mov	x2, sp
>  	bl	do_mem_abort
> -- 
> 2.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] arm64: documentation: document tagged pointer stack constraints

[Kristina Martsenko]

Some kernel features don't currently work if a task puts a non-zero
address tag in its stack pointer, frame pointer, or frame record entries
(FP, LR).

For example, with a tagged stack pointer, the kernel can't deliver
signals to the process, and the task is killed instead. As another
example, with a tagged frame pointer or frame records, perf fails to
generate call graphs or resolve symbols.

For now, just document these limitations, instead of finding and fixing
everything that doesn't work, as it's not known if anyone needs to use
tags in these places anyway.

In addition, as requested by Dave Martin, generalize the limitations
into a general kernel address tag policy, and refactor
tagged-pointers.txt to include it.

Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
---
 Documentation/arm64/tagged-pointers.txt | 62 +++++++++++++++++++++++++--------
 1 file changed, 47 insertions(+), 15 deletions(-)

diff --git a/Documentation/arm64/tagged-pointers.txt b/Documentation/arm64/tagged-pointers.txt
index d9995f1f51b3..a25a99e82bb1 100644
--- a/Documentation/arm64/tagged-pointers.txt
+++ b/Documentation/arm64/tagged-pointers.txt
@@ -11,24 +11,56 @@ in AArch64 Linux.
 The kernel configures the translation tables so that translations made
 via TTBR0 (i.e. userspace mappings) have the top byte (bits 63:56) of
 the virtual address ignored by the translation hardware. This frees up
-this byte for application use, with the following caveats:
+this byte for application use.
 
-	(1) The kernel requires that all user addresses passed to EL1
-	    are tagged with tag 0x00. This means that any syscall
-	    parameters containing user virtual addresses *must* have
-	    their top byte cleared before trapping to the kernel.
 
-	(2) Non-zero tags are not preserved when delivering signals.
-	    This means that signal handlers in applications making use
-	    of tags cannot rely on the tag information for user virtual
-	    addresses being maintained for fields inside siginfo_t.
-	    One exception to this rule is for signals raised in response
-	    to watchpoint debug exceptions, where the tag information
-	    will be preserved.
+Passing tagged addresses to the kernel
+--------------------------------------
 
-	(3) Special care should be taken when using tagged pointers,
-	    since it is likely that C compilers will not hazard two
-	    virtual addresses differing only in the upper byte.
+All interpretation of userspace memory addresses by the kernel assumes
+an address tag of 0x00.
+
+This includes, but is not limited to, addresses found in:
+
+ - pointer arguments to system calls, including pointers in structures
+   passed to system calls,
+
+ - the stack pointer (sp), e.g. when interpreting it to deliver a
+   signal,
+
+ - the frame pointer (x29) and frame records, e.g. when interpreting
+   them to generate a backtrace or call graph.
+
+Using non-zero address tags in any of these locations may result in an
+error code being returned, a (fatal) signal being raised, or other modes
+of failure.
+
+For these reasons, passing non-zero address tags to the kernel via
+system calls is forbidden, and using a non-zero address tag for sp is
+strongly discouraged.
+
+Programs maintaining a frame pointer and frame records that use non-zero
+address tags may suffer impaired or inaccurate debug and profiling
+visibility.
+
+
+Preserving tags
+---------------
+
+Non-zero tags are not preserved when delivering signals. This means that
+signal handlers in applications making use of tags cannot rely on the
+tag information for user virtual addresses being maintained for fields
+inside siginfo_t. One exception to this rule is for signals raised in
+response to watchpoint debug exceptions, where the tag information will
+be preserved.
 
 The architecture prevents the use of a tagged PC, so the upper byte will
 be set to a sign-extension of bit 55 on exception return.
+
+
+Other considerations
+--------------------
+
+Special care should be taken when using tagged pointers, since it is
+likely that C compilers will not hazard two virtual addresses differing
+only in the upper byte.
-- 
2.1.4

[PATCH v2 0/4] arm64: improve tagged pointer handling

[Will Deacon]

Hi Kristina,

On Wed, May 03, 2017 at 04:37:44PM +0100, Kristina Martsenko wrote:
> Here are some patches to fix a few issues related to tagged pointer
> handling.

These all look good to me, thanks:

Acked-by: Will Deacon <will.deacon@arm.com>

Catalin -- I think you can take these all as fixes, with CC stable too
(fixes: tags are already in place).

Cheers,

Will

[PATCH v2 0/4] arm64: improve tagged pointer handling

[Catalin Marinas]

On Wed, May 03, 2017 at 04:37:44PM +0100, Kristina Martsenko wrote:
> Kristina Martsenko (4):
>   arm64: traps: fix userspace cache maintenance emulation on a tagged
>     pointer
>   arm64: hw_breakpoint: fix watchpoint matching for tagged pointers
>   arm64: entry: improve data abort handling of tagged pointers
>   arm64: documentation: document tagged pointer stack constraints

Queued for 4.12. Thanks.

-- 
Catalin