On Wed, Oct 16, 2019 at 6:29 AM Will Deacon will@kernel.org wrote:
On Tue, Oct 15, 2019 at 05:14:53PM +0100, Will Deacon wrote:
On Tue, Oct 15, 2019 at 04:26:51PM +0100, Catalin Marinas wrote:
On Mon, Oct 14, 2019 at 10:33:32PM +0100, Will Deacon wrote:
On Mon, Oct 14, 2019 at 05:26:51PM +0100, Catalin Marinas wrote:
The options I see:
Revert commit 057d3389108e and try again to document that the memory syscalls do not support tagged pointers
Change untagged_addr() to only 0-extend from bit 55 or leave the tag unchanged if bit 55 is 1. We could mask out the tag (0 rather than sign-extend) but if we had an mlock test passing ULONG_MASK, then we get -ENOMEM instead of -EINVAL
Make untagged_addr() depend on the TIF_TAGGED_ADDR bit and we only break the ABI for applications opting in to this new ABI. We did look at this but the ptrace(PEEK/POKE_DATA) needs a bit more thinking on whether we check the ptrace'd process or the debugger flags
Leave things as they are, consider the address space 56-bit and change the test to not use LONG_MAX on arm64. This needs to be passed by the sparc guys since they probably have a similar issue
I'm in favour of (2) or (4) as long as there's also an update to the docs.
With (4) we'd start differing from other architectures supported by Linux. This works if we consider the test to be broken. However, reading the mlock man page:
EINVAL The result of the addition addr+len was less than addr (e.g., the addition may have resulted in an overflow). ENOMEM Some of the specified address range does not correspond to mapped pages in the address space of the process.There is no mention of EINVAL outside the TASK_SIZE, seems to fall more within the ENOMEM description above.
Sorry, I was being too vague in my wording. What I was trying to say is I'm ok with (2) or (4), but either way we need to update our ABI documentation under Documentation/arm64/.
Having looked at making that change, I actually think the text is ok as-is if we go with option (2). We only make guarantees about "valid tagged pointer", which are defined to "reference an address in the user process address space" and therefore must have bit 55 == 0.
Untested patch below.
Will
--->8
From 517d979e84191ae9997c9513a88a5b798af6912f Mon Sep 17 00:00:00 2001 From: Will Deacon will@kernel.org Date: Tue, 15 Oct 2019 21:04:18 -0700 Subject: [PATCH] arm64: tags: Preserve tags for addresses translated via TTBR1
Sign-extending TTBR1 addresses when converting to an untagged address breaks the documented POSIX semantics for mlock() in some obscure error cases where we end up returning -EINVAL instead of -ENOMEM as a direct result of rewriting the upper address bits.
Rework the untagged_addr() macro to preserve the upper address bits for TTBR1 addresses and only clear the tag bits for user addresses. This matches the behaviour of the 'clear_address_tag' assembly macro, so rename that and align the implementations at the same time so that they use the same instruction sequences for the tag manipulation.
Cc: Catalin Marinas catalin.marinas@arm.com Link: https://lore.kernel.org/stable/20191014162651.GF19200@arrakis.emea.arm.com/ Reported-by: Jan Stancek jstancek@redhat.com Signed-off-by: Will Deacon will@kernel.org
Reviewed-by: Andrey Konovalov andreyknvl@google.com
Thanks!
arch/arm64/include/asm/asm-uaccess.h | 7 +++---- arch/arm64/include/asm/memory.h | 10 ++++++++-- arch/arm64/kernel/entry.S | 4 ++-- 3 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h index f74909ba29bd..5bf963830b17 100644 --- a/arch/arm64/include/asm/asm-uaccess.h +++ b/arch/arm64/include/asm/asm-uaccess.h @@ -78,10 +78,9 @@ alternative_else_nop_endif /*
- Remove the address tag from a virtual address, if present.
*/
.macro clear_address_tag, dst, addrtst \addr, #(1 << 55)bic \dst, \addr, #(0xff << 56)csel \dst, \dst, \addr, eq
.macro untagged_addr, dst, addrsbfx \dst, \addr, #0, #56and \dst, \dst, \addr .endm#endif diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h index b61b50bf68b1..c23c47360664 100644 --- a/arch/arm64/include/asm/memory.h +++ b/arch/arm64/include/asm/memory.h @@ -215,12 +215,18 @@ static inline unsigned long kaslr_offset(void)
- 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) \ +#define __untagged_addr(addr) \ ((__force __typeof__(addr))sign_extend64((__force u64)(addr), 55))
+#define untagged_addr(addr) ({ \
u64 __addr = (__force u64)addr; \__addr &= __untagged_addr(__addr); \(__force __typeof__(addr))__addr; \+})
#ifdef CONFIG_KASAN_SW_TAGS #define __tag_shifted(tag) ((u64)(tag) << 56) -#define __tag_reset(addr) untagged_addr(addr) +#define __tag_reset(addr) __untagged_addr(addr) #define __tag_get(addr) (__u8)((u64)(addr) >> 56) #else #define __tag_shifted(tag) 0UL diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index e304fe04b098..9ae336cc5833 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -604,7 +604,7 @@ el1_da: */ mrs x3, far_el1 inherit_daif pstate=x23, tmp=x2
clear_address_tag x0, x3
untagged_addr x0, x3 mov x2, sp // struct pt_regs bl do_mem_abort@@ -808,7 +808,7 @@ el0_da: mrs x26, far_el1 ct_user_exit_irqoff enable_daif
clear_address_tag x0, x26
untagged_addr x0, x26 mov x1, x25 mov x2, sp bl do_mem_abort-- 2.23.0.700.g56cf767bdb-goog