[scudo] Refactor of CRC32 and ARM runtime CRC32 detection

[cryptoad]

Summary

ARM & AArch64 runtime detection for hardware support of CRC32 has been added
via check of the AT_HWVAL auxiliary vector.

Following Michal's suggestions in D28417, the CRC32 code has been further
changed and looks better now. When compiled with full relro (which is strongly
suggested to benefit from additional hardening), the weak symbol for
computeHardwareCRC32 is read-only and the assembly generated is fairly clean
and straight forward. As suggested, an additional optimization is to skip
the runtime check if SSE 4.2 has been enabled globally, as opposed to only
for scudo_crc32.cpp.

scudo_crc32.h has no purpose anymore and was removed.

[rengolin]

lib/scudo/scudo_allocator.cpp

87

nit pick, an extra else adds clarity and helps future changes.

lib/scudo/scudo_utils.cpp

105

This only works on Linux, right?

[cryptoad]

lib/scudo/scudo_utils.cpp

105

Correct. This is ensured in config-ix.cmake:

if (COMPILER_RT_HAS_SANITIZER_COMMON AND SCUDO_SUPPORTED_ARCH AND
    OS_NAME MATCHES "Linux")
  set(COMPILER_RT_HAS_SCUDO TRUE)
else()
  set(COMPILER_RT_HAS_SCUDO FALSE)
endif()

I can add a check for SANITIZER_LINUX being defined in the top header if you feel it makes things clearer.

[rengolin]

Did you test this on AArch32 and AArch64?

lib/scudo/scudo_utils.cpp

105

Probably a good idea, if anyone ever updates the CMake file. :)

[cryptoad]

Tested locally on AArch64 and armhf to be exact.
AArch64 is not yet supported in the the cmake config, the patch is next.

In D28574#645510, @rengolin wrote:

Did you test this on AArch32 and AArch64?

[cryptoad]

Addressing review comments.

[rengolin]

LGTM. Thanks!

[cryptoad]

Thank you Renato! I will wait for @mgorny's comments to make sure it fits his idea as well.

[mgorny]

LGTM modulo the first comment. I won't be able to test it today, though, but I doubt it could break something for me ;-).

lib/scudo/scudo_allocator.cpp

82

If I'm not mistaken, you could add || defined(__ARM_FEATURE_CRC32) to cover the same idea for ARM.

85

I'm sorry if I'm a bit ignorant here but why are you checking the function address? Is there a potential case for it being null?

[cryptoad]

lib/scudo/scudo_allocator.cpp

82

I think for ARM, since the feature could be enabled via -mcrc (specifically for CRC32, unlike the whole instruction set for SSE 4.2), the runtime check is still required, as no other potentially illegal instructions would be emitted anywhere else.

85

Given that it's a weak symbol, if not implemented anywhere the pointer will be null indeed (some details can be found at https://en.wikipedia.org/wiki/Weak_symbol) after linking. That way, if -msse42 is not available for scudo_crc32.cpp, computeHardwareCRC32 remains unimplemented, and we fallback to the software implementation.

[rengolin]

lib/scudo/scudo_allocator.cpp

82

Right, this is actually a good point. There are basically two ideas here:

1. You compile to force one implementation or the other, via flags. In this case, if you compile to have CRC, it will fail with sigill on cores that don't have it. The most likely scenario is that all builds will end up not having it, for safety, so you'll never use it.

2. You have a run time check, as it is now, so that it's not a compilation option and you'll always use whenever you can. The problem now is that you end up paying the comparison price on every call.

The third, and best (but more complicated) way is to use IFUNC, but that's certainly for another patch.

FWIW, right now, the AARch64 sanitizers are getting the VMA configuration via run time calls, just like this patch, because of the problems outlines above, so this is not without prior art. :)

[mgorny]

lib/scudo/scudo_allocator.cpp

82

However, the instruction can be enabled implicitly via -march. If you build purely for ARM target that supports CRC32, I don't see a point not to use it.

85

Oh, that was the case I was missing. I don't mind using this then. However, personally I would do it the more obvious way and just #ifdef for the case when hardware CRC32 would not be implemented instead of checking at runtime ;-).

[rengolin]

lib/scudo/scudo_allocator.cpp

82

With ARM, the probability that you build in one machine (type) and run in another is higher than in x86. The number of variations and hardware support is also much much higher.

All x86_64 chips of family X have feature Y. A mix bag of ARM chips on family X, which support feature Y, has it in a way that is actually usable (ie. some cores report the feature but you really rather not use it).

All I'm saying is that, using -march=aarch64+crc will not work on any core that doesn't have it, so everyone will *not* use it in case the binary has to run on hardware that doesn't have it. Any Linux distribution (including Android) will not enable it for that reason.

If you're advocating for hard-coding if the feature IS there in mattr (either +crc or +nocrc), but leaving run-time if it's not, than I think that'll be ok, as it's a platform choice.

[cryptoad]

lib/scudo/scudo_allocator.cpp

82

IIUC, the option suggested by Michal is the counterpart to SSE 4.2: test for -mcrc support in the compiler, add it to scudo_crc32.cpp only if detected and keep the runtime check in that case. In the event of -march with CRC support, scudo_allocator.cpp can bypass the runtime check and just do hardware.

85

I had issues with #ifdef as only the .cpp is compiled with CRC32, leaving me without any symbol I could check for existence.
If you have a suggestion around that, I'll take it :)

[rengolin]

lib/scudo/scudo_allocator.cpp

82

Sounds good.

[mgorny]

lib/scudo/scudo_allocator.cpp

85

I meant making CMake explicitly define some value after determining that SSE4.2/CRC32 support is available. But that's a task for a separate patch, I guess.

lib/scudo/scudo_crc32.cpp

28

One more thing: don't you want to enable -mcrc via CMake for this file?

[cryptoad]

lib/scudo/scudo_allocator.cpp

85

Understood!

lib/scudo/scudo_crc32.cpp

28

I wasn't planning to with this, but I will update the patch to take this into account.

[cryptoad]

Adding the cmake changes to allow -mcrc detection and use.

During additional testing, it was discovered that HWCAP_CRC32 was not defined
on some platforms.

[phosek]

lib/scudo/scudo_utils.cpp

110

This is likely going to create a dependency on libc++ similarly to D28757.

[cryptoad]

lib/scudo/scudo_utils.cpp

110

Changing that. Is calling getauxval OK?

[cryptoad]

Removing the static qualifier for the HWCap variable to prevent libc++
dependencies.

[phosek]

lib/scudo/scudo_utils.cpp

110

Yes that's fine, it's the static initialization that's the problem.

[cryptoad]

@mgorny @rengolin I would appreciate if I could get a final nod regarding the latest changes to this patch. Thanks!

[mgorny]

Still LGTM. I presume you have tested the final version ;-).

lib/scudo/scudo_allocator.h

 
 #ifndef SCUDO_ALLOCATOR_H_
 #define SCUDO_ALLOCATOR_H_
 
 #include "scudo_flags.h"
 
 #include "sanitizer_common/sanitizer_allocator.h"
 
+#if !SANITIZER_LINUX
+# error "The Scudo hardened allocator is currently only supported on Linux."
+#endif
+
 #include <atomic>
 
 namespace __scudo {
 
 enum AllocType : u8 {
   FromMalloc    = 0, // Memory block came from malloc, realloc, calloc, etc.
   FromNew       = 1, // Memory block came from operator new.
   FromNewArray  = 2, // Memory block came from operator new [].

lib/scudo/scudo_allocator.cpp

 /// Scudo Hardened Allocator implementation.
 /// It uses the sanitizer_common allocator as a base and aims at mitigating
 /// heap corruption vulnerabilities. It provides a checksum-guarded chunk
 /// header, a delayed free list, and additional sanity checks.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "scudo_allocator.h"
-#include "scudo_crc32.h"
 #include "scudo_utils.h"
 
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_quarantine.h"
 
 #include <limits.h>
 #include <pthread.h>
 

 static thread_local Xorshift128Plus Prng;
 // Global static cookie, initialized at start-up.
 static uptr Cookie;
 
 // We default to software CRC32 if the alternatives are not supported, either
 // at compilation or at runtime.
 static atomic_uint8_t HashAlgorithm = { CRC32Software };
 
-// Helper function that will compute the chunk checksum, being passed all the
-// the needed information as uptrs. It will opt for the hardware version of
-// the checksumming function if available.
-INLINE u32 hashUptrs(uptr Pointer, uptr *Array, uptr ArraySize, u8 HashType) {
-  u32 Crc;
-  Crc = computeCRC32(Cookie, Pointer, HashType);
-  for (uptr i = 0; i < ArraySize; i++)
-    Crc = computeCRC32(Crc, Array[i], HashType);
-  return Crc;
+SANITIZER_WEAK_ATTRIBUTE u32 computeHardwareCRC32(u32 Crc, uptr Data);
+
+INLINE u32 computeCRC32(u32 Crc, uptr Data, u8 HashType) {
+  // If SSE4.2 is defined here, it was enabled everywhere, as opposed to only
+  // for scudo_crc32.cpp. This means that other SSE instructions were likely
+  // emitted at other places, and as a result there is no reason to not use
+  // the hardware version of the CRC32.
+#if defined(__SSE4_2__)
+  return computeHardwareCRC32(Crc, Data);
+#else
+  if (computeHardwareCRC32 && HashType == CRC32Hardware)
+    return computeHardwareCRC32(Crc, Data);
+  else
+    return computeSoftwareCRC32(Crc, Data);
+#endif  // defined(__SSE4_2__)
 }
 
 struct ScudoChunk : UnpackedHeader {
   // We can't use the offset member of the chunk itself, as we would double
   // fetch it without any warranty that it wouldn't have been tampered. To
   // prevent this, we work with a local copy of the header.
   void *getAllocBeg(UnpackedHeader *Header) {
     return reinterpret_cast<void *>(
struct ScudoChunk : UnpackedHeader {
   }
 
   // Compute the checksum of the Chunk pointer and its ChunkHeader.
   u16 computeChecksum(UnpackedHeader *Header) const {
     UnpackedHeader ZeroChecksumHeader = *Header;
     ZeroChecksumHeader.Checksum = 0;
     uptr HeaderHolder[sizeof(UnpackedHeader) / sizeof(uptr)];
     memcpy(&HeaderHolder, &ZeroChecksumHeader, sizeof(HeaderHolder));
-    u32 Hash = hashUptrs(reinterpret_cast<uptr>(this),
-                         HeaderHolder,
-                         ARRAY_SIZE(HeaderHolder),
-                         atomic_load_relaxed(&HashAlgorithm));
-    return static_cast<u16>(Hash);
+    u8 HashType = atomic_load_relaxed(&HashAlgorithm);
+    u32 Crc = computeCRC32(Cookie, reinterpret_cast<uptr>(this), HashType);
+    for (uptr i = 0; i < ARRAY_SIZE(HeaderHolder); i++)
+      Crc = computeCRC32(Crc, HeaderHolder[i], HashType);
+    return static_cast<u16>(Crc);
   }
 
   // Checks the validity of a chunk by verifying its checksum.
   bool isValid() {
     UnpackedHeader NewUnpackedHeader;
     const AtomicPackedHeader *AtomicHeader =
         reinterpret_cast<const AtomicPackedHeader *>(this);
     PackedHeader NewPackedHeader =

lib/scudo/scudo_crc32.h

-//===-- scudo_crc32.h -------------------------------------------*- C++ -*-===//
-//
-//                     The LLVM Compiler Infrastructure
-//
-// This file is distributed under the University of Illinois Open Source
-// License. See LICENSE.TXT for details.
-//
-//===----------------------------------------------------------------------===//
-///
-/// Header for scudo_crc32.cpp.
-///
-//===----------------------------------------------------------------------===//
-
-#ifndef SCUDO_CRC32_H_
-#define SCUDO_CRC32_H_
-
-#include "sanitizer_common/sanitizer_internal_defs.h"
-
-namespace __scudo {
-
-enum : u8 {
-  CRC32Software = 0,
-  CRC32Hardware = 1,
-};
-
-u32 computeCRC32(u32 Crc, uptr Data, u8 HashType);
-
-}  // namespace __scudo
-
-#endif  // SCUDO_CRC32_H_

lib/scudo/scudo_crc32.cpp

 //===-- scudo_crc32.cpp -----------------------------------------*- C++ -*-===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// CRC32 function leveraging hardware specific instructions. This has to be
 /// kept separated to restrict the use of compiler specific flags to this file.
 ///
 //===----------------------------------------------------------------------===//
 
+#include "sanitizer_common/sanitizer_internal_defs.h"
+
 // Hardware CRC32 is supported at compilation via the following:
 // - for i386 & x86_64: -msse4.2
 // - for ARM & AArch64: -march=armv8-a+crc or -mcrc
 // An additional check must be performed at runtime as well to make sure the
 // emitted instructions are valid on the target host.
-#include "scudo_crc32.h"
-#include "scudo_utils.h"
 
 #if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 # ifdef __SSE4_2__
 #  include <smmintrin.h>
 #  define CRC32_INTRINSIC FIRST_32_SECOND_64(_mm_crc32_u32, _mm_crc32_u64)
 # endif
 # ifdef __ARM_FEATURE_CRC32
 #  include <arm_acle.h>
 #  define CRC32_INTRINSIC FIRST_32_SECOND_64(__crc32cw, __crc32cd)
 # endif
 #endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
 namespace __scudo {
 
 #if defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
-INLINE u32 computeHardwareCRC32(u32 Crc, uptr Data) {
+u32 computeHardwareCRC32(u32 Crc, uptr Data) {
   return CRC32_INTRINSIC(Crc, Data);
 }
-
-u32 computeCRC32(u32 Crc, uptr Data, u8 HashType) {
-  if (HashType == CRC32Hardware) {
-    return computeHardwareCRC32(Crc, Data);
-  }
-  return computeSoftwareCRC32(Crc, Data);
-}
-#else
-u32 computeCRC32(u32 Crc, uptr Data, u8 HashType) {
-  return computeSoftwareCRC32(Crc, Data);
-}
 #endif  // defined(__SSE4_2__) || defined(__ARM_FEATURE_CRC32)
 
 }  // namespace __scudo

lib/scudo/scudo_utils.h

u64 Next() {
     x ^= x << 23;
     State[1] = x ^ y ^ (x >> 17) ^ (y >> 26);
     return State[1] + y;
   }
  private:
   u64 State[2];
 };
 
-// Software CRC32 functions, to be used when hardware support is not detected.
+enum : u8 {
+  CRC32Software = 0,
+  CRC32Hardware = 1,
+};
+
 u32 computeSoftwareCRC32(u32 Crc, uptr Data);
 
 }  // namespace __scudo
 
 #endif  // SCUDO_UTILS_H_

lib/scudo/scudo_utils.cpp

 
 #include <errno.h>
 #include <fcntl.h>
 #include <stdarg.h>
 #include <unistd.h>
 #if defined(__x86_64__) || defined(__i386__)
 # include <cpuid.h>
 #endif
-
-#include <cstring>
+#if defined(__arm__) || defined(__aarch64__)
+# include <sys/auxv.h>
+#endif
 
 // TODO(kostyak): remove __sanitizer *Printf uses in favor for our own less
 //                complicated string formatting code. The following is a
 //                temporary workaround to be able to use __sanitizer::VSNPrintf.
 namespace __sanitizer {
 
 extern int VSNPrintf(char *buff, int buff_length, const char *format,
                      va_list args);
bool testCPUFeature(CPUFeature Feature)
   switch (Feature) {
     case CRC32CPUFeature:  // CRC32 is provided by SSE 4.2.
       return !!(FeaturesRegs.Ecx & bit_SSE4_2);
     default:
       break;
   }
   return false;
 }
+#elif defined(__arm__) || defined(__aarch64__)
+// For ARM and AArch64, hardware CRC32 support is indicated in the
+// AT_HWVAL auxiliary vector.
+bool testCPUFeature(CPUFeature Feature) {
+  static uptr HWCap = getauxval(AT_HWCAP);
+
+  switch (Feature) {
+    case CRC32CPUFeature:
+      return !!(HWCap & HWCAP_CRC32);
+    default:
+      break;
+  }
+  return false;
+}
 #else
 bool testCPUFeature(CPUFeature Feature) {
   return false;
 }
 #endif  // defined(__x86_64__) || defined(__i386__)
 
 // readRetry will attempt to read Count bytes from the Fd specified, and if
 // interrupted will retry to read additional bytes to reach Count.