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

[PATCH 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


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               |  4 ++-
 arch/arm64/kernel/hw_breakpoint.c       |  3 ++
 arch/arm64/kernel/traps.c               |  4 +--
 6 files changed, 67 insertions(+), 21 deletions(-)

-- 
2.1.4

[PATCH 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 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 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            | 4 +++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
index df411f3e083c..790ce8e64f8d 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, addr, tmp
+	bic	\tmp, \addr, #(0xff << 56)
+	tst	\addr, #(1 << 55)
+	csel	\addr, \tmp, \addr, eq
+	.endm
+
 #endif
diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
index 43512d4d7df2..2f7ec392ef50 100644
--- a/arch/arm64/kernel/entry.S
+++ b/arch/arm64/kernel/entry.S
@@ -434,6 +434,7 @@ el1_da:
 	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,8 @@ el0_da:
 	// enable interrupts before calling the main handler
 	enable_dbg_and_irq
 	ct_user_exit
-	bic	x0, x26, #(0xff << 56)
+	mov	x0, x26
+	clear_address_tag x0, x3
 	mov	x1, x25
 	mov	x2, sp
 	bl	do_mem_abort
-- 
2.1.4

[PATCH 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.

Cc: 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 1/4] arm64: traps: fix userspace cache maintenance emulation on a tagged pointer

[Andre Przywara]

Hi Kristina,

On 20/04/17 19:17, Kristina Martsenko wrote:
> 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.

Right, well spotted!
So thanks for the patch, which I think is correct. But ...

> 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.

Looking more closely at this code, I see that we actually don't use the
address parameter in the force_signal_inject() function. Instead we
always put the PC address into the siginfo structure, which is wrong in
case this SEGV is triggered by an invalid address of a cache maintenance
operation.

I made a simple patch to fix this (using the address argument and
explicitly passing the PC in when we fault on an invalid instruction).

But now we would pass the untagged address back into userland. I am not
sure this is a real problem, since we don't promise anything in case of
tagged pointers, if I got this correctly.

But also our untagged_addr() macro seems to not cover all cases
correctly, for instance passing in 0x00ffffffffff5678 (which is an
invalid, but untagged address) would extend to some probably valid
kernel pointer. And although this would fail our user space address
check, we would return a wrong address (with all the upper bits being 1)
in siginfo.

Do we care about this?
What would be the best fix for the untagged_addr macro? Is that macro
actually the proper place to fix this issue?

Cheers,
Andre.

> 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 */
> 

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

[Dave P Martin]

On Thu, Apr 20, 2017 at 07:17:14PM +0100, Kristina Martsenko wrote:
> 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.
>
> Cc: Dave Martin <dave.martin@arm.com>
> Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>

Looks OK to me.  It's not feasible to describe all possible scenarios
here, but this makes the kernel's intentions clearer.

Reviewed-by: Dave Martin <Dave.Martin@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
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

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

[Dave Martin]

On Thu, Apr 20, 2017 at 07:17:13PM +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.

Possibly it doesn't matter whether the tag bits are cleared for an EL0
fault on a TTBR1 address, since userspace can't have a valid pointer in
this range to (mis)match the fault address against ... or did I miss
something?

Factoring out the tag handling makes the intent of the code clearer
though, either way.

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            | 4 +++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h
> index df411f3e083c..790ce8e64f8d 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, addr, tmp
> +	bic	\tmp, \addr, #(0xff << 56)
> +	tst	\addr, #(1 << 55)
> +	csel	\addr, \tmp, \addr, eq
> +	.endm
> +
>  #endif
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 43512d4d7df2..2f7ec392ef50 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -434,6 +434,7 @@ el1_da:
>  	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,8 @@ el0_da:
>  	// enable interrupts before calling the main handler
>  	enable_dbg_and_irq
>  	ct_user_exit
> -	bic	x0, x26, #(0xff << 56)
> +	mov	x0, x26
> +	clear_address_tag x0, x3
>  	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 1/4] arm64: traps: fix userspace cache maintenance emulation on a tagged pointer

[Kristina Martsenko]

Hi Andre,

On 21/04/17 11:59, Andre Przywara wrote:
> On 20/04/17 19:17, Kristina Martsenko wrote:
>> 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.
> 
> Right, well spotted!
> So thanks for the patch, which I think is correct. But ...

Thanks for taking a look.

>> 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.
> 
> Looking more closely at this code, I see that we actually don't use the
> address parameter in the force_signal_inject() function. Instead we
> always put the PC address into the siginfo structure, which is wrong in
> case this SEGV is triggered by an invalid address of a cache maintenance
> operation.

I agree this is a bug in existing code, as it means we currently put a
different address into the siginfo structure when we emulate cache
maintenance, compared to when we don't emulate it.

I think the bug is independent of this series though, and the fix should
be sent as a separate patch/series, as it is not blocking this series
and doesn't involve address tags.

> I made a simple patch to fix this (using the address argument and
> explicitly passing the PC in when we fault on an invalid instruction).

Sounds about right to me. Although I noticed that swp emulation also
goes through arm64_notify_segfault. I think the behaviour for swp
emulation should stay the same as in arch/arm/ set_segfault(), i.e.
faulting address used in find_vma, but PC passed in siginfo. Unless
that's a bug in arch/arm/ (for a non-emulated swp, they seem to pass the
faulting address in siginfo instead).

> But now we would pass the untagged address back into userland. I am not
> sure this is a real problem, since we don't promise anything in case of
> tagged pointers, if I got this correctly.

Yes, as documented in Documentation/arm64/tagged-pointers.txt, we do not
guarantee that the tag is preserved when delivering a signal.

> But also our untagged_addr() macro seems to not cover all cases
> correctly, for instance passing in 0x00ffffffffff5678 (which is an
> invalid, but untagged address) would extend to some probably valid
> kernel pointer. And although this would fail our user space address
> check, we would return a wrong address (with all the upper bits being 1)
> in siginfo.
> 
> Do we care about this?
> What would be the best fix for the untagged_addr macro? Is that macro
> actually the proper place to fix this issue?

I don't know if we care, but personally I think that if
force_signal_inject is fixed, then untagged_addr should be fixed along
with it. I think the macro is the right place to fix it, by only sign
extending if bit 55 is 0. That way it will turn a tagged address into an
untagged address, and will not change a non-tagged address. (This is
also what clear_address_tag in patch #3 of this series does.)

Thanks,
Kristina

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

[Kristina Martsenko]

On 21/04/17 19:24, Dave Martin wrote:
> On Thu, Apr 20, 2017 at 07:17:13PM +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.
> 
> Possibly it doesn't matter whether the tag bits are cleared for an EL0
> fault on a TTBR1 address, since userspace can't have a valid pointer in
> this range to (mis)match the fault address against ... or did I miss
> something?

I don't think you've missed anything. But I don't see why userspace
can't match against an invalid (TTBR1) address, I think that would be a
valid thing to do (even if unlikely).

> Factoring out the tag handling makes the intent of the code clearer
> though, either way.

I assume this means you're fine with the patch as is.

Thanks,
Kristina

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

[Dave Martin]

On Thu, Apr 27, 2017 at 05:34:24PM +0100, Kristina Martsenko wrote:
> On 21/04/17 19:24, Dave Martin wrote:
> > On Thu, Apr 20, 2017 at 07:17:13PM +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.
> > 
> > Possibly it doesn't matter whether the tag bits are cleared for an EL0
> > fault on a TTBR1 address, since userspace can't have a valid pointer in
> > this range to (mis)match the fault address against ... or did I miss
> > something?
> 
> I don't think you've missed anything. But I don't see why userspace
> can't match against an invalid (TTBR1) address, I think that would be a
> valid thing to do (even if unlikely).
> 
> > Factoring out the tag handling makes the intent of the code clearer
> > though, either way.
> 
> I assume this means you're fine with the patch as is.

Yes, the behaviour here seems fine.  I just wanted to check there wasn't
some real use of TTBR1 addresses by userspace that I was missing.


It looks like a minor improvement may be possible (below) but these
aren't hot paths, so it's far from crucial.

> +/*
> + * Remove the address tag from a virtual address, if present.
> + */
> +	.macro	clear_address_tag, addr, tmp
> +	bic	\tmp, \addr, #(0xff << 56)
> +	tst	\addr, #(1 << 55)
> +	csel	\addr, \tmp, \addr, eq
> +	.endm

Minor nit: this may issue better on simpler microarchitectures if the
tst and bic are swapped: this may reduce how far the Z flag needs to
be forwarded.

For beefier microarchitectures it probably doesn't make a difference.

Also, in:

> @@ -594,7 +595,8 @@ el0_da:
>  	// enable interrupts before calling the main handler
>  	enable_dbg_and_irq
>  	ct_user_exit
> -	bic	x0, x26, #(0xff << 56)
> +	mov	x0, x26
> +	clear_address_tag x0, x3

we can lose the mov if clear_address_tag is altered to take "addr out"
and "addr in" arguments instead of "addr in+out" and "tmp": addr out can
store result of the bic, then we can conditionally revert that back to
addr in using the csel.

This change will break el1_da:

> @@ -434,6 +434,7 @@ el1_da:
> 	mrs	x0, 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

... because we want the same register as input and output.  But that can
be fixed if we use a different destination register in the mrs in the
first place (say, x3).

Cheers
---Dave