From b19f584f2c1001ac9e4af3750fdc84d5bd53f6ca Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 20 Jun 2023 23:01:43 +0100 Subject: [PATCH 01/15] Fix for arm64_32 (aka ILP32) on Clang Signed-off-by: Dave Rodgman --- library/constant_time.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index c823b78894..6e02f34389 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -78,8 +78,10 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi uint32_t r; #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); -#elif defined(__aarch64__) +#elif defined(__aarch64__) && (SIZE_MAX == 0xffffffffffffffff) asm volatile ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); +#elif defined(__aarch64__) && (SIZE_MAX == 0xffffffff) + asm volatile ("ldr %w0, [%w1]" : "=r" (r) : "r" (p) :); #endif return r; } From 04cb9ac59ea0943b298ccb1f3db3e4d97da40dff Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 07:32:22 +0100 Subject: [PATCH 02/15] Fix for arm64_32 (aka ILP32) on Clang (attempt 2) Signed-off-by: Dave Rodgman --- library/constant_time.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index 6e02f34389..b24ebb4787 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -78,10 +78,8 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi uint32_t r; #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); -#elif defined(__aarch64__) && (SIZE_MAX == 0xffffffffffffffff) - asm volatile ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); -#elif defined(__aarch64__) && (SIZE_MAX == 0xffffffff) - asm volatile ("ldr %w0, [%w1]" : "=r" (r) : "r" (p) :); +#elif defined(__aarch64__) + asm volatile ("ldr %w0, [%1]" : "=r" (r) : "p" (p) :); #endif return r; } From b67db9140ede4bf7b5f1a2bab8a31f119941f360 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 09:15:27 +0100 Subject: [PATCH 03/15] Separate ILP32 and normal-aarch64 code paths Signed-off-by: Dave Rodgman --- library/constant_time.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/library/constant_time.c b/library/constant_time.c index b24ebb4787..89d7b4f234 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -79,7 +79,12 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); #elif defined(__aarch64__) +#if (SIZE_MAX == 0xffffffff) + /* ILP32: Specify the pointer operand slightly differently, as per #7787. */ asm volatile ("ldr %w0, [%1]" : "=r" (r) : "p" (p) :); +#else + asm volatile ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); +#endif #endif return r; } From 517e891e5550cb889c5374bd8dad7e26e9f3245e Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 11:16:31 +0100 Subject: [PATCH 04/15] Changelog Signed-off-by: Dave Rodgman --- ChangeLog.d/fix-ilp32.txt | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 ChangeLog.d/fix-ilp32.txt diff --git a/ChangeLog.d/fix-ilp32.txt b/ChangeLog.d/fix-ilp32.txt new file mode 100644 index 0000000000..7c28d68f43 --- /dev/null +++ b/ChangeLog.d/fix-ilp32.txt @@ -0,0 +1,4 @@ +Bugfix + * Fix a compile failure in the constant_time module when building + for watchos (i.e. for Aarch64 ILP32). Reported by Paulo Coutinho + in #7787. From 85842b8edb1f3cf18cb525f068d9c105b09563fe Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 11:22:09 +0100 Subject: [PATCH 05/15] Be strict about pointer size in mbedtls_get_unaligned_volatile_uint32 Signed-off-by: Dave Rodgman --- library/constant_time.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index 89d7b4f234..fb14c9cf3c 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -63,7 +63,8 @@ * only used here. */ #if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) -#if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) || defined(__aarch64__) +#if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && (SIZE_MAX == 0xffffffff)) || \ + (defined(__aarch64__) && ((SIZE_MAX == 0xffffffff) || (SIZE_MAX == 0xffffffffffffffff))) #define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS #endif #endif From 63e89b46f8b6cd603bb2b73e58545f940aae9a85 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 11:55:17 +0100 Subject: [PATCH 06/15] Use UINTPTR_MAX not SIZE_MAX Signed-off-by: Dave Rodgman --- library/constant_time.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index fb14c9cf3c..f7da39f8e1 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -63,8 +63,9 @@ * only used here. */ #if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) -#if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && (SIZE_MAX == 0xffffffff)) || \ - (defined(__aarch64__) && ((SIZE_MAX == 0xffffffff) || (SIZE_MAX == 0xffffffffffffffff))) +#if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && (UINTPTR_MAX == 0xfffffffful)) || \ + (defined(__aarch64__) && ((UINTPTR_MAX == 0xffffffffull) || (UINTPTR_MAX == 0xffffffffffffffffull))) +/* We check pointer sizes to avoid issues with them not matching register size requirements */ #define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS #endif #endif @@ -80,10 +81,11 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); #elif defined(__aarch64__) -#if (SIZE_MAX == 0xffffffff) +#if (UINTPTR_MAX == 0xfffffffful) /* ILP32: Specify the pointer operand slightly differently, as per #7787. */ asm volatile ("ldr %w0, [%1]" : "=r" (r) : "p" (p) :); -#else +#elif (UINTPTR_MAX == 0xffffffffffffffffull) + /* aarch64 with 64-bit pointers */ asm volatile ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); #endif #endif From 140fa15a7f8f56a05fda6d9c8ed6286b5bdaba27 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 12:36:52 +0100 Subject: [PATCH 07/15] Improve changelog Signed-off-by: Dave Rodgman --- ChangeLog.d/fix-ilp32.txt | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ChangeLog.d/fix-ilp32.txt b/ChangeLog.d/fix-ilp32.txt index 7c28d68f43..3f18ac5c5b 100644 --- a/ChangeLog.d/fix-ilp32.txt +++ b/ChangeLog.d/fix-ilp32.txt @@ -1,4 +1,4 @@ Bugfix - * Fix a compile failure in the constant_time module when building - for watchos (i.e. for Aarch64 ILP32). Reported by Paulo Coutinho - in #7787. + * Fix a compilation failure in the constant_time module when + building for arm64_32 (e.g., for watchos). Reported by Paulo + Coutinho in #7787. From c54f25e26cf1a2a44f78fd1bac08a1c35c691fd0 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 13:39:30 +0100 Subject: [PATCH 08/15] code style Signed-off-by: Dave Rodgman --- library/constant_time.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index f7da39f8e1..5ed087c07a 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -63,8 +63,10 @@ * only used here. */ #if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) -#if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && (UINTPTR_MAX == 0xfffffffful)) || \ - (defined(__aarch64__) && ((UINTPTR_MAX == 0xffffffffull) || (UINTPTR_MAX == 0xffffffffffffffffull))) +#if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && \ + (UINTPTR_MAX == 0xfffffffful)) || \ + (defined(__aarch64__) && ((UINTPTR_MAX == 0xffffffffull) || \ + (UINTPTR_MAX == 0xffffffffffffffffull))) /* We check pointer sizes to avoid issues with them not matching register size requirements */ #define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS #endif From 0400ae2f9b2a146acf77436daf7aee0e14101b84 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 16:14:46 +0100 Subject: [PATCH 09/15] Fix pointer constraint in bn_mul.h Signed-off-by: Dave Rodgman --- library/bn_mul.h | 5 ++++- library/common.h | 18 ++++++++++++++++++ library/constant_time.c | 8 +------- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/library/bn_mul.h b/library/bn_mul.h index c5994f7049..0af7ecddec 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -265,7 +265,10 @@ "str x5, [%1], #8 \n\t" #define MULADDC_X1_STOP \ - : "+r" (c), "+r" (d), "+r" (s), "+m" (*(uint64_t (*)[16]) d) \ + : "+r" (c), \ + "+" MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (d), \ + "+" MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (s), \ + "+m" (*(uint64_t (*)[16]) d) \ : "r" (b), "m" (*(const uint64_t (*)[16]) s) \ : "x4", "x5", "x6", "x7", "cc" \ ); diff --git a/library/common.h b/library/common.h index b48a1fc667..4ee183a3f9 100644 --- a/library/common.h +++ b/library/common.h @@ -169,6 +169,24 @@ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned #endif /* *INDENT-ON* */ +/* + * Define the constraint used for pointer operands to asm. + * + * This is normally the usual "r", but for aarch64_32 (aka ILP32, + * as found in watchos), "p" is required to avoid warnings from clang. + */ +#if defined(__aarch64__) && defined(MBEDTLS_HAVE_ASM) +#if UINTPTR_MAX == 0xfffffffful +/* ILP32: Specify the pointer operand slightly differently, as per #7787. */ +#define MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT "p" +#elif UINTPTR_MAX == 0xfffffffffffffffful +/* Normal case (64-bit pointers): use "r" as the constraint for pointer operands to asm */ +#define MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT "r" +#else +#error Unrecognised pointer size for aarch64 +#endif +#endif + /* Always provide a static assert macro, so it can be used unconditionally. * It will expand to nothing on some systems. * Can be used outside functions (but don't add a trailing ';' in that case: diff --git a/library/constant_time.c b/library/constant_time.c index 5ed087c07a..c62ec13816 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -83,13 +83,7 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); #elif defined(__aarch64__) -#if (UINTPTR_MAX == 0xfffffffful) - /* ILP32: Specify the pointer operand slightly differently, as per #7787. */ - asm volatile ("ldr %w0, [%1]" : "=r" (r) : "p" (p) :); -#elif (UINTPTR_MAX == 0xffffffffffffffffull) - /* aarch64 with 64-bit pointers */ - asm volatile ("ldr %w0, [%1]" : "=r" (r) : "r" (p) :); -#endif + asm volatile ("ldr %w0, [%1]" : "=r" (r) : MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (p) :); #endif return r; } From b5b6939fc29187aa6d87395bf4b898f9bf0105b1 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 16:36:42 +0100 Subject: [PATCH 10/15] Remove redundant checks in constant_time.c Signed-off-by: Dave Rodgman --- library/constant_time.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/library/constant_time.c b/library/constant_time.c index c62ec13816..5e1a5773ee 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -64,9 +64,7 @@ */ #if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS) && defined(MBEDTLS_HAVE_ASM) #if ((defined(__arm__) || defined(__thumb__) || defined(__thumb2__)) && \ - (UINTPTR_MAX == 0xfffffffful)) || \ - (defined(__aarch64__) && ((UINTPTR_MAX == 0xffffffffull) || \ - (UINTPTR_MAX == 0xffffffffffffffffull))) + (UINTPTR_MAX == 0xfffffffful)) || defined(__aarch64__) /* We check pointer sizes to avoid issues with them not matching register size requirements */ #define MBEDTLS_EFFICIENT_UNALIGNED_VOLATILE_ACCESS #endif From 5b5dd011d109bb4c1d5cc1edae119ea3889ce412 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 16:36:47 +0100 Subject: [PATCH 11/15] code style Signed-off-by: Dave Rodgman --- library/constant_time.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/constant_time.c b/library/constant_time.c index 5e1a5773ee..2faba69e45 100644 --- a/library/constant_time.c +++ b/library/constant_time.c @@ -81,7 +81,7 @@ static inline uint32_t mbedtls_get_unaligned_volatile_uint32(volatile const unsi #if defined(__arm__) || defined(__thumb__) || defined(__thumb2__) asm volatile ("ldr %0, [%1]" : "=r" (r) : "r" (p) :); #elif defined(__aarch64__) - asm volatile ("ldr %w0, [%1]" : "=r" (r) : MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (p) :); + asm volatile ("ldr %w0, [%1]" : "=r" (r) : MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT(p) :); #endif return r; } From e6c9996d04b8b52ca4d33fbfc221024673252968 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Wed, 21 Jun 2023 21:16:23 +0100 Subject: [PATCH 12/15] Work around updating pointers from ILP32 Signed-off-by: Dave Rodgman --- library/bn_mul.h | 10 +++++----- library/common.h | 3 +++ 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/library/bn_mul.h b/library/bn_mul.h index 0af7ecddec..93dd4b6bb5 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -248,10 +248,10 @@ #endif /* AMD64 */ -#if defined(__aarch64__) +#if defined(__aarch64__) && (UINTPTR_MAX == 0xfffffffful || UINTPTR_MAX == 0xfffffffffffffffful) #define MULADDC_X1_INIT \ - asm( + do { uintptr_t muladdc_d = (uintptr_t) d, muladdc_s = (uintptr_t) s; asm( #define MULADDC_X1_CORE \ "ldr x4, [%2], #8 \n\t" \ @@ -266,12 +266,12 @@ #define MULADDC_X1_STOP \ : "+r" (c), \ - "+" MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (d), \ - "+" MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT (s), \ + "+r" (muladdc_d), \ + "+r" (muladdc_s), \ "+m" (*(uint64_t (*)[16]) d) \ : "r" (b), "m" (*(const uint64_t (*)[16]) s) \ : "x4", "x5", "x6", "x7", "cc" \ - ); + ); d = (mbedtls_mpi_uint *)muladdc_d; s = (mbedtls_mpi_uint *)muladdc_s; } while (0); #endif /* Aarch64 */ diff --git a/library/common.h b/library/common.h index 4ee183a3f9..ba9cb75c08 100644 --- a/library/common.h +++ b/library/common.h @@ -174,6 +174,9 @@ inline void mbedtls_xor(unsigned char *r, const unsigned char *a, const unsigned * * This is normally the usual "r", but for aarch64_32 (aka ILP32, * as found in watchos), "p" is required to avoid warnings from clang. + * + * Note that clang does not recognise '+p' or '=p', and armclang + * does not recognise 'p' at all. */ #if defined(__aarch64__) && defined(MBEDTLS_HAVE_ASM) #if UINTPTR_MAX == 0xfffffffful From 4e5c63d65248f06b704a51fe794b473f41ba247d Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Fri, 23 Jun 2023 15:17:37 +0100 Subject: [PATCH 13/15] Improve documentation in bn_mul.h Co-authored-by: Tom Cosgrove Signed-off-by: Dave Rodgman --- library/bn_mul.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/library/bn_mul.h b/library/bn_mul.h index 93dd4b6bb5..4ccd7b4b17 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -248,6 +248,8 @@ #endif /* AMD64 */ +// The following assembly code assumes that a pointer will fit in a 64-bit register +// (including ILP32 __aarch64__ ABIs such as on watchOS, hence the 2^32 - 1) #if defined(__aarch64__) && (UINTPTR_MAX == 0xfffffffful || UINTPTR_MAX == 0xfffffffffffffffful) #define MULADDC_X1_INIT \ From 9e868be13a04887b02c2b13fcaeb8d97a28abbd6 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 27 Jun 2023 09:27:27 +0100 Subject: [PATCH 14/15] Fix clang warning from -Wasm-operand-widths Signed-off-by: Dave Rodgman --- library/bn_mul.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/library/bn_mul.h b/library/bn_mul.h index 4ccd7b4b17..95265a4d06 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -256,15 +256,15 @@ do { uintptr_t muladdc_d = (uintptr_t) d, muladdc_s = (uintptr_t) s; asm( #define MULADDC_X1_CORE \ - "ldr x4, [%2], #8 \n\t" \ - "ldr x5, [%1] \n\t" \ + "ldr x4, [%x2], #8 \n\t" \ + "ldr x5, [%x1] \n\t" \ "mul x6, x4, %4 \n\t" \ "umulh x7, x4, %4 \n\t" \ "adds x5, x5, x6 \n\t" \ "adc x7, x7, xzr \n\t" \ "adds x5, x5, %0 \n\t" \ "adc %0, x7, xzr \n\t" \ - "str x5, [%1], #8 \n\t" + "str x5, [%x1], #8 \n\t" #define MULADDC_X1_STOP \ : "+r" (c), \ From 8c5fae2610fe10d687b6453de036478e906fefa4 Mon Sep 17 00:00:00 2001 From: Dave Rodgman Date: Tue, 27 Jun 2023 09:43:55 +0100 Subject: [PATCH 15/15] Add explanatory comment Signed-off-by: Dave Rodgman --- library/bn_mul.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/library/bn_mul.h b/library/bn_mul.h index 95265a4d06..43dd5c298f 100644 --- a/library/bn_mul.h +++ b/library/bn_mul.h @@ -252,6 +252,13 @@ // (including ILP32 __aarch64__ ABIs such as on watchOS, hence the 2^32 - 1) #if defined(__aarch64__) && (UINTPTR_MAX == 0xfffffffful || UINTPTR_MAX == 0xfffffffffffffffful) +/* + * There are some issues around different compilers requiring different constraint + * syntax for updating pointers from assembly code (see notes for + * MBEDTLS_ASM_AARCH64_PTR_CONSTRAINT in common.h), especially on aarch64_32 (aka ILP32). + * + * For this reason we cast the pointers to/from uintptr_t here. + */ #define MULADDC_X1_INIT \ do { uintptr_t muladdc_d = (uintptr_t) d, muladdc_s = (uintptr_t) s; asm(