Remove alignment requirement on CRYPTO_poly1305_finish.

This dates to https://boringssl-review.googlesource.com/2850, which was
done in response to an ARM crash. I assume the ARM crash was due to
poly1305_arm.c casting pointers around, which is technically UB, even on
x86 since C says it is UB to cast pointers if the value would be
unaligned. (Also I believe it's a strict aliasing violation, though the
compilers really ought to give us a sanitizer for it if they're excited
about that optimization.)

Replace with memcpy, which any reasonable compiler would compile the
same on platforms that support unaligned access. ARM does support it
these days, so perhaps the crash came from an older ARM?

Benchmarks showed no difference with this CL.

Change-Id: I022bdb84f95e45c143ad19359f646ee1416d5ae9
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/39344
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>

crypto/poly1305/poly1305_arm.c

@@ -103,7 +103,17 @@ static void freeze(fe1305x2 *r) {
   r->v[8] = y4;
 }
 
-static void fe1305x2_tobytearray(uint8_t *r, fe1305x2 *x) {
+static void store32(uint8_t out[4], uint32_t v) { OPENSSL_memcpy(out, &v, 4); }
+
+// load32 exists to avoid breaking strict aliasing rules in
+// fe1305x2_frombytearray.
+static uint32_t load32(const uint8_t t[4]) {
+  uint32_t tmp;
+  OPENSSL_memcpy(&tmp, t, sizeof(tmp));
+  return tmp;
+}
+
+static void fe1305x2_tobytearray(uint8_t r[16], fe1305x2 *x) {
   uint32_t x0 = x->v[0];
   uint32_t x1 = x->v[2];
   uint32_t x2 = x->v[4];
@@ -119,22 +129,13 @@ static void fe1305x2_tobytearray(uint8_t *r, fe1305x2 *x) {
   x4 += x3 >> 26;
   x3 &= 0x3ffffff;
 
-  *(uint32_t *)r = x0 + (x1 << 26);
-  *(uint32_t *)(r + 4) = (x1 >> 6) + (x2 << 20);
-  *(uint32_t *)(r + 8) = (x2 >> 12) + (x3 << 14);
-  *(uint32_t *)(r + 12) = (x3 >> 18) + (x4 << 8);
+  store32(r, x0 + (x1 << 26));
+  store32(r + 4, (x1 >> 6) + (x2 << 20));
+  store32(r + 8, (x2 >> 12) + (x3 << 14));
+  store32(r + 12, (x3 >> 18) + (x4 << 8));
 }
 
-// load32 exists to avoid breaking strict aliasing rules in
-// fe1305x2_frombytearray.
-static uint32_t load32(uint8_t *t) {
-  uint32_t tmp;
-  OPENSSL_memcpy(&tmp, t, sizeof(tmp));
-  return tmp;
-}
-
-static void fe1305x2_frombytearray(fe1305x2 *r, const uint8_t *x,
-                                   unsigned long long xlen) {
+static void fe1305x2_frombytearray(fe1305x2 *r, const uint8_t *x, size_t xlen) {
   unsigned i;
   uint8_t t[17];
 
@@ -190,11 +191,11 @@ void CRYPTO_poly1305_init_neon(poly1305_state *state, const uint8_t key[32]) {
   fe1305x2 *const precomp = c + 1;
   unsigned int j;
 
-  r->v[1] = r->v[0] = 0x3ffffff & *(uint32_t *)key;
-  r->v[3] = r->v[2] = 0x3ffff03 & ((*(uint32_t *)(key + 3)) >> 2);
-  r->v[5] = r->v[4] = 0x3ffc0ff & ((*(uint32_t *)(key + 6)) >> 4);
-  r->v[7] = r->v[6] = 0x3f03fff & ((*(uint32_t *)(key + 9)) >> 6);
-  r->v[9] = r->v[8] = 0x00fffff & ((*(uint32_t *)(key + 12)) >> 8);
+  r->v[1] = r->v[0] = 0x3ffffff & load32(key);
+  r->v[3] = r->v[2] = 0x3ffff03 & (load32(key + 3) >> 2);
+  r->v[5] = r->v[4] = 0x3ffc0ff & (load32(key + 6) >> 4);
+  r->v[7] = r->v[6] = 0x3f03fff & (load32(key + 9) >> 6);
+  r->v[9] = r->v[8] = 0x00fffff & (load32(key + 12) >> 8);
 
   for (j = 0; j < 10; j++) {
     h->v[j] = 0;  // XXX: should fast-forward a bit

crypto/poly1305/poly1305_test.cc

@@ -62,8 +62,7 @@ static void TestSIMD(unsigned excess, const std::vector<uint8_t> &key,
   // Consume the remainder and finish.
   CRYPTO_poly1305_update(&state, in.data() + done, in.size() - done);
 
-  // |CRYPTO_poly1305_finish| requires a 16-byte-aligned output.
-  alignas(16) uint8_t out[16];
+  uint8_t out[16];
   CRYPTO_poly1305_finish(&state, out);
   EXPECT_EQ(Bytes(out), Bytes(mac)) << "SIMD pattern " << excess << " failed.";
 }
@@ -81,8 +80,7 @@ TEST(Poly1305Test, TestVectors) {
     poly1305_state state;
     CRYPTO_poly1305_init(&state, key.data());
     CRYPTO_poly1305_update(&state, in.data(), in.size());
-    // |CRYPTO_poly1305_finish| requires a 16-byte-aligned output.
-    alignas(16) uint8_t out[16];
+    uint8_t out[16];
     CRYPTO_poly1305_finish(&state, out);
     EXPECT_EQ(Bytes(out), Bytes(mac)) << "Single-shot Poly1305 failed.";
 
@@ -94,6 +92,17 @@ TEST(Poly1305Test, TestVectors) {
     CRYPTO_poly1305_finish(&state, out);
     EXPECT_EQ(Bytes(out), Bytes(mac)) << "Streaming Poly1305 failed.";
 
+    // Test |CRYPTO_poly1305_init| and |CRYPTO_poly1305_finish| work on
+    // unaligned values.
+    alignas(8) uint8_t unaligned_key[32 + 1];
+    OPENSSL_memcpy(unaligned_key + 1, key.data(), 32);
+    CRYPTO_poly1305_init(&state, unaligned_key + 1);
+    CRYPTO_poly1305_update(&state, in.data(), in.size());
+    alignas(8) uint8_t unaligned_out[16 + 1];
+    CRYPTO_poly1305_finish(&state, unaligned_out + 1);
+    EXPECT_EQ(Bytes(unaligned_out + 1, 16), Bytes(mac))
+        << "Unaligned Poly1305 failed.";
+
     // Test SIMD stress patterns. OpenSSL's AVX2 assembly needs a multiple of
     // four blocks, so test up to three blocks of excess.
     TestSIMD(0, key, in, mac);

crypto/poly1305/poly1305_vec.c

@@ -27,9 +27,21 @@
 
 #include <emmintrin.h>
 
-#define U8TO64_LE(m) (*(const uint64_t *)(m))
-#define U8TO32_LE(m) (*(const uint32_t *)(m))
-#define U64TO8_LE(m, v) (*(uint64_t *)(m)) = v
+static uint32_t load_u32_le(const uint8_t in[4]) {
+  uint32_t ret;
+  OPENSSL_memcpy(&ret, in, 4);
+  return ret;
+}
+
+static uint64_t load_u64_le(const uint8_t in[8]) {
+  uint64_t ret;
+  OPENSSL_memcpy(&ret, in, 8);
+  return ret;
+}
+
+static void store_u64_le(uint8_t out[8], uint64_t v) {
+  OPENSSL_memcpy(out, &v, 8);
+}
 
 typedef __m128i xmmi;
 
@@ -96,8 +108,8 @@ void CRYPTO_poly1305_init(poly1305_state *state, const uint8_t key[32]) {
   uint64_t t0, t1;
 
   // clamp key
-  t0 = U8TO64_LE(key + 0);
-  t1 = U8TO64_LE(key + 8);
+  t0 = load_u64_le(key + 0);
+  t1 = load_u64_le(key + 8);
   r0 = t0 & 0xffc0fffffff;
   t0 >>= 44;
   t0 |= t1 << 20;
@@ -115,10 +127,10 @@ void CRYPTO_poly1305_init(poly1305_state *state, const uint8_t key[32]) {
   p->R22.d[3] = (uint32_t)(r2 >> 32);
 
   // store pad
-  p->R23.d[1] = U8TO32_LE(key + 16);
-  p->R23.d[3] = U8TO32_LE(key + 20);
-  p->R24.d[1] = U8TO32_LE(key + 24);
-  p->R24.d[3] = U8TO32_LE(key + 28);
+  p->R23.d[1] = load_u32_le(key + 16);
+  p->R23.d[3] = load_u32_le(key + 20);
+  p->R24.d[1] = load_u32_le(key + 24);
+  p->R24.d[3] = load_u32_le(key + 28);
 
   // H = 0
   st->H[0] = _mm_setzero_si128();
@@ -750,8 +762,8 @@ void CRYPTO_poly1305_finish(poly1305_state *state, uint8_t mac[16]) {
   }
 
 poly1305_donna_atleast16bytes:
-  t0 = U8TO64_LE(m + 0);
-  t1 = U8TO64_LE(m + 8);
+  t0 = load_u64_le(m + 0);
+  t1 = load_u64_le(m + 8);
   h0 += t0 & 0xfffffffffff;
   t0 = shr128_pair(t1, t0, 44);
   h1 += t0 & 0xfffffffffff;
@@ -790,8 +802,8 @@ poly1305_donna_atmost15bytes:
   OPENSSL_memset(m + leftover, 0, 16 - leftover);
   leftover = 16;
 
-  t0 = U8TO64_LE(m + 0);
-  t1 = U8TO64_LE(m + 8);
+  t0 = load_u64_le(m + 0);
+  t1 = load_u64_le(m + 8);
   h0 += t0 & 0xfffffffffff;
   t0 = shr128_pair(t1, t0, 44);
   h1 += t0 & 0xfffffffffff;
@@ -837,8 +849,8 @@ poly1305_donna_finish:
   t1 = (t1 >> 24);
   h2 += (t1)+c;
 
-  U64TO8_LE(mac + 0, ((h0) | (h1 << 44)));
-  U64TO8_LE(mac + 8, ((h1 >> 20) | (h2 << 24)));
+  store_u64_le(mac + 0, ((h0) | (h1 << 44)));
+  store_u64_le(mac + 8, ((h1 >> 20) | (h2 << 24)));
 }
 
 #endif  // !OPENSSL_WINDOWS && OPENSSL_X86_64

include/openssl/poly1305.h

@@ -28,19 +28,17 @@ typedef uint8_t poly1305_state[512];
 // authentication tag with the one-time key |key|. Note that |key| is a
 // one-time key and therefore there is no `reset' method because that would
 // enable several messages to be authenticated with the same key.
-OPENSSL_EXPORT void CRYPTO_poly1305_init(poly1305_state* state,
+OPENSSL_EXPORT void CRYPTO_poly1305_init(poly1305_state *state,
                                          const uint8_t key[32]);
 
 // CRYPTO_poly1305_update processes |in_len| bytes from |in|. It can be called
 // zero or more times after poly1305_init.
-OPENSSL_EXPORT void CRYPTO_poly1305_update(poly1305_state* state,
-                                           const uint8_t* in,
-                                           size_t in_len);
+OPENSSL_EXPORT void CRYPTO_poly1305_update(poly1305_state *state,
+                                           const uint8_t *in, size_t in_len);
 
 // CRYPTO_poly1305_finish completes the poly1305 calculation and writes a 16
-// byte authentication tag to |mac|. The |mac| address must be 16-byte
-// aligned.
-OPENSSL_EXPORT void CRYPTO_poly1305_finish(poly1305_state* state,
+// byte authentication tag to |mac|.
+OPENSSL_EXPORT void CRYPTO_poly1305_finish(poly1305_state *state,
                                            uint8_t mac[16]);