Issue Building MbedTLS 3.6.0 on Fedora

[tom-cosgrove-arm]

(reported by @billatarm)

The compiler is complaining about an array index out of bounds in mbedtls_xor on the trailing portion that handles the leftover bytes that are beyond a multiple of the neon lane width. The gcc version is 140001.

We tested the brain dead version of a byte for byte loop and with the optimizer turned on we got neon instructions as expected at -O2.

See below for details

Fedora builds with:

export CFLAGS='-mbranch-protection=standard -fasynchronous-unwind-tables -fstack-clash-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -Wno-stringop-overflow -Wno-maybe-uninitialized -Wall -Wextra -Wwrite-strings -Wformat=2 -Wno-format-nonliteral -Wvla -Wlogical-op -Wshadow -Wformat-signedness -Wformat-overflow=2 -Wformat-truncation -Werror'

In file included from /home/user/workspace/mbedtls/library/ctr_drbg.c:13:
In function ‘mbedtls_xor’,
    inlined from ‘ctr_drbg_update_internal’ at /home/user/workspace/mbedtls/library/ctr_drbg.c:372:5:
/home/user/workspace/mbedtls/library/common.h:235:17: error: array subscript 48 is outside array bounds of ‘unsigned char[48]’ [-Werror=array-bounds=]
  235 |         r[i] = a[i] ^ b[i];
      |                ~^~~
/home/user/workspace/mbedtls/library/ctr_drbg.c: In function ‘ctr_drbg_update_internal’:
/home/user/workspace/mbedtls/library/ctr_drbg.c:335:19: note: at offset 48 into object ‘tmp’ of size 48
  335 |     unsigned char tmp[MBEDTLS_CTR_DRBG_SEEDLEN];
      |                   ^~~
In function ‘mbedtls_xor’,
    inlined from ‘ctr_drbg_update_internal’ at /home/user/workspace/mbedtls/library/ctr_drbg.c:372:5:
/home/user/workspace/mbedtls/library/common.h:235:24: error: array subscript 48 is outside array bounds of ‘const unsigned char[48]’ [-Werror=array-bounds=]
  235 |         r[i] = a[i] ^ b[i];
      |                       ~^~~
/home/user/workspace/mbedtls/library/ctr_drbg.c: In function ‘ctr_drbg_update_internal’:
/home/user/workspace/mbedtls/library/ctr_drbg.c:333:57: note: at offset 48 into object ‘data’ of size [0, 48]
  333 |                                     const unsigned char data[MBEDTLS_CTR_DRBG_SEEDLEN])
      |                                     ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In function ‘mbedtls_xor’,
    inlined from ‘ctr_drbg_update_internal’ at /home/user/workspace/mbedtls/library/ctr_drbg.c:372:5:
/home/user/workspace/mbedtls/library/common.h:235:14: error: array subscript 48 is outside array bounds of ‘unsigned char[48]’ [-Werror=array-bounds=]
  235 |         r[i] = a[i] ^ b[i];
      |         ~~~~~^~~~~~~~~~~~~
/home/user/workspace/mbedtls/library/ctr_drbg.c: In function ‘ctr_drbg_update_internal’:
/home/user/workspace/mbedtls/library/ctr_drbg.c:335:19: note: at offset 48 into object ‘tmp’ of size 48
  335 |     unsigned char tmp[MBEDTLS_CTR_DRBG_SEEDLEN];
      |                   ^~~

[billatarm]

FYI this is on Fedora Rawhide

[daverodgman]

For reference, #8922 fixes some fairly similar warnings so maybe similar approach can be used

[rany2]

I'm not sure if you're aware of this or if this is helpful, but the following change fixes it on my machine:

diff --git a/library/common.h b/library/common.h
index 3936ffdfe..d8c407319 100644
--- a/library/common.h
+++ b/library/common.h
@@ -192,21 +192,21 @@ static inline void mbedtls_xor(unsigned char *r,
 #if defined(MBEDTLS_EFFICIENT_UNALIGNED_ACCESS)
 #if defined(MBEDTLS_HAVE_NEON_INTRINSICS) && \
     (!(defined(MBEDTLS_COMPILER_IS_GCC) && MBEDTLS_GCC_VERSION < 70300))
     /* Old GCC versions generate a warning here, so disable the NEON path for these compilers */
     for (; (i + 16) <= n; i += 16) {
         uint8x16_t v1 = vld1q_u8(a + i);
         uint8x16_t v2 = vld1q_u8(b + i);
         uint8x16_t x = veorq_u8(v1, v2);
         vst1q_u8(r + i, x);
     }
-#if defined(__IAR_SYSTEMS_ICC__)
+#if defined(__IAR_SYSTEMS_ICC__) || defined(MBEDTLS_COMPILER_IS_GCC)
     /* This if statement helps some compilers (e.g., IAR) optimise out the byte-by-byte tail case
      * where n is a constant multiple of 16.
      * For other compilers (e.g. recent gcc and clang) it makes no difference if n is a compile-time
      * constant, and is a very small perf regression if n is not a compile-time constant. */
     if (n % 16 == 0) {
         return;
     }
 #endif
 #elif defined(MBEDTLS_ARCH_IS_X64) || defined(MBEDTLS_ARCH_IS_ARM64)
     /* This codepath probably only makes sense on architectures with 64-bit registers */

[mortenstevens]

It works now after applying the patch from @rany2 Fedora buildlogs: https://koji.fedoraproject.org/koji/buildinfo?buildID=2451607

[rossburton]

Can the use of -Werror just be made optional? Constant whack-a-mole with new compilers does get boring.

[billatarm]

Can the use of -Werror just be made optional? Constant whack-a-mole with new compilers does get boring.

I'd rather play whack-a-mole, on the off chance a new error is detected that is a serious bug, I'd rather avoid shipping the update.