[PATCH] D28417: [scudo] Separate hardware CRC32 routines

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad added inline comments.


================
Comment at: lib/scudo/scudo_crc32.cpp:42
+u32 computeCRC32(u32 Crc, uptr Data, u8 HashType) {
+  if (HashType == CRC32Hardware) {
+    return computeHardwareCRC32(Crc, Data);
----------------
mgorny wrote:
> This is still not entirely correct. Strictly speaking, the compiler can emit SSE4.2 instructions here, i.e. making them apply to pre-hardware or non-hardware call branch. However, it's highly unlikely it will use SSE4.2 to do comparison or function call, so I'm not going to insist on moving this elsewhere.
> 
> However, indirection into two function calls may actually affect performance, I think. Since hardware and software variants have the same signature, maybe it'd be better to replace the enum with function pointer, store hardware or software function in it and just call that function pointer in scudo_allocator?
> 
> Also, this reminds me I wanted to point out that ARM hardware variant is never used since hardware function is only called when SSE4.2 is available.
Thank you Michal for your comments, I appreciate you having a look.

You are right about the function pointer, it would make things easier. The reasoning behind ot using one was that a call to a writeable function pointer at the core of the allocator could be leveraged by an attacker for arbitrary code execution purposes - which we are trying to avoid. Hence me trying to jump through hoops to avoid that.

Regarding the code generation, I tried to re-arrange things different ways without be able to get rid of the calls. If a solution jumps at you, I'll be happy to change things.

As for ARM, the reason there is no hardware path available yet is only due to me not having the correct devices to test it properly yet. I have an ARM32 board (without CRC capabilities), and just managed to get an AArch64 compliant OS on a Raspberry Pi 3, so it will get there shortly. I managed to get things working with -march=armv8.1-a+crc, and a runtime check based on getauxval(AT_HWCAP).


https://reviews.llvm.org/D28417