Thread-safety (non-?)issue in mbedtls_aesni_has_support()

[solardiz]

I notice there's a thread-safety (non-?)issue in mbedtls_aesni_has_support(), where the done variable may be set before c even though they're set in the other order in the C source. The compiler is free to re-order. The effect would be that another thread could run non-AES-NI code (perhaps just once) even on an AES-NI capable CPU. As long as either code is indeed correct and they're compatible in terms of context struct content, this should be a non-issue. But are they compatible? Not sure.

A compiler memory barrier may need to be added before the done = 1; line. And another before reading from c, as the reads may also be re-ordered otherwise. Like this:

+++ b/src/mbedtls/aesni.c
@@ -67,9 +67,11 @@ int mbedtls_aesni_has_support(unsigned int what)
              :
              : "eax", "ebx", "edx");
 #endif /* MBEDTLS_AESNI_HAVE_CODE */
+        asm ("" ::: "memory");
         done = 1;
     }
 
+    asm ("" ::: "memory");
     return (c & what) != 0;
 }
 #endif /* !MBEDTLS_AES_USE_HARDWARE_ONLY */

Curiously, this reduced code size a tiny bit for me.

Since this code is x86-only, I assume we can rely on x86's strict memory ordering, without introducing hardware memory barriers.

[gilles-peskine-arm]

Good point. IIRC the AESNI assembly, AESNI with intrinsics and our pure-software implementation use the same context layout (it's pretty natural for a “naive” AES implementation), but we should check this.

Also of note, mbedtls_aesce_has_support_impl in aesce.c follows the same pattern and has the same bug.

[solardiz]

Also of note, mbedtls_aesce_has_support_impl in aesce.c follows the same pattern and has the same bug.

I had actually looked at it and no, I think it does not have the same bug - it uses just one variable, not two. So it may perform the checks more than once (in different threads), but with the same results (each thread will choose the same AES code version).

... and this suggests a different way to fix mbedtls_aesni_has_support() - switch it to using just one variable as well, move the flag check inside the do-once portion.