On Wed, Oct 16, 2019 at 05:29:33AM +0100, Will Deacon 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
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, addr
- tst \addr, #(1 << 55)
- bic \dst, \addr, #(0xff << 56)
- csel \dst, \dst, \addr, eq
- .macro untagged_addr, dst, addr
- sbfx \dst, \addr, #0, #56
- and \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) ({ \
Having the same informal name ("untagged") for two different address representations seems like a recipe for confusion. Can we rename one of them? (Say, untagged_address_if_user()?)
- u64 __addr = (__force u64)addr; \
Missing () round addr.
Also, nit: needlessly fragile macro? (OK, callers are unlikely to pass "__addr" for addr, but the __addr variable doesn't do a lot here other than to avoid repeated evaluation of the argument -- I don't expect this to matter given how this macro is used.)
- __addr &= __untagged_addr(__addr); \
- (__force __typeof__(addr))__addr; \
+})
Is there any reason why we can't just have
#define untagged_addr ((__force __typeof__(addr))( \ (__force u64)(addr) & GENMASK_ULL(63, 56)))
?
Either way, "kernel" addresses (bit 55 set) become unusable garbage, but we _want_ such addresses passed from userspace to be unusable. Comparison against TASK_SIZE will still police them accurately.
Simply zero-extending would be a less obfuscated way of only ever rounding the address down -- it's the rounding up that spuriously triggers address wraparound and leads to the -EINVAL return.
(Tests for error codes are inherently fragile, and MTE requires POSIX wording to be interpreted in a context not anticipated by the authors -- so I'm still not totally convinced we need a band-aid for this. But anyway...)
[...]
Cheers ---Dave