Power 8 AES implementation using built-ins

Question

I need a review of an AltiVec/Power 8 AES implementation using built-ins to access in-core crypto. The program below is an implementation of FIPS 197, Appendix B. The hard-coded values are due to the fixed key and message from the appendix.

The program is correct, but it may have some room for improvement. PowerPC and Power 8 is not our area of expertise, so there could be a lot of room for improvement. The code was tested on GCC112, which is a little endian machine running Linux. It was also tested on GCC119, which is a big endian machine running AIX. Both machines provide GCC and IBM's XL C/C++.

I had to work around several XL C/C++ bugs. That's why oddities like this are present:

uint8x16_p8 Load8x16(const uint8_t src[16])
{
#if defined(__xlc__) || defined(__xlC__)
    /* IBM XL C/C++ compiler */
    uint8_t* s = (uint8_t*)src;
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
    return vec_xl_be(0, s);
# else
    return vec_xl(0, s);
# endif
#else
    /* GCC, Clang, etc */
    return (uint8x16_p8)vec_vsx_ld(0, src);
#endif
}

The requirements are:

1. GCC and IBM XL C/C++ compiler support
2. Big-endian and little-endian support
3. The input key can be unaligned. The user supplies it.
4. The input message can be unaligned. The user supplies it.
5. The output buffer can be unaligned. The user supplies it.
6. The subkeys must be 16-byte aligned to allow use of vec_ld. The library controls them, so this is easy to comply with.

The program is available at Noloader/AES-Power8 github. It is reproduced below.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdint.h>

#if defined(__ALTIVEC__)
# include <altivec.h>
#endif

#if defined(__xlc__) || defined(__xlC__)
// #include <builtins.h>
typedef vector unsigned char uint8x16_p8;
typedef vector unsigned int uint64x2_p8;
#elif defined(__GNUC__)
typedef vector unsigned char uint8x16_p8;
typedef vector unsigned long long uint64x2_p8;
#endif

/* Avoid 0 == 0 when __BYTE_ORDER__ is not defined */
#ifndef __BYTE_ORDER__
# define __BYTE_ORDER__ -16
#endif

uint8x16_p8 Load8x16(const uint8_t src[16])
{
#if defined(__xlc__) || defined(__xlC__)
   /* IBM XL C/C++ compiler */
   uint8_t* s = (uint8_t*)src;
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   return vec_xl_be(0, s);
# else
   return vec_xl(0, s);
# endif
#else
   /* GCC, Clang, etc */
   return (uint8x16_p8)vec_vsx_ld(0, src);
#endif
}

void Save8x16(const uint8x16_p8 src, uint8_t dest[16])
{
#if defined(__xlc__) || defined(__xlC__)
   /* IBM XL C/C++ compiler */
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   vec_xst_be(src, 0, dest);
# else
   vec_xst(src, 0, dest);
# endif
#else
   /* GCC, Clang, etc */
   vec_vsx_st(src, 0, dest);
#endif
}

uint64x2_p8 Load64x2(const uint8_t src[16])
{
#if defined(__xlc__) || defined(__xlC__)
   /* IBM XL C/C++ compiler */
   uint8_t* s = (uint8_t*)src;
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   return (uint64x2_p8)vec_xl_be(0, s);
# else
   return (uint64x2_p8)vec_xl(0, s);
# endif
#else
   /* GCC, Clang, etc */
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   __attribute__((aligned(16)))
   static const uint8_t m[16] =
      /* Looks a lot like an epi8 mask ... */
      { 15,14,13,12,  11,10,9,8,  7,6,5,4, 3,2,1,0 };
   __attribute__((aligned(16)))
   static const uint8_t z[16] =
      { 0,0,0,0,  0,0,0,0,  0,0,0,0,  0,0,0,0 };

   const uint8x16_p8 mask = vec_ld(0, m);
   const uint8x16_p8 zero = vec_ld(0, z);

   return (uint64x2_p8)vec_perm(Load8x16(src), zero, mask);
# else
   return (uint64x2_p8)vec_vsx_ld(0, src);
# endif
#endif
}

void Save64x2(const uint64x2_p8 src, uint8_t dest[16])
{
#if defined(__xlc__) || defined(__xlC__)
   /* IBM XL C/C++ compiler */
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   vec_xst_be((uint8x16_p8)src, 0, dest);
# else
   vec_xst((uint8x16_p8)src, 0, dest);
# endif
#else
   /* GCC, Clang, etc */
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   __attribute__((aligned(16)))
   static const uint8_t m[16] =
      /* Looks a lot like an epi8 mask ... */
      { 15,14,13,12,  11,10,9,8,  7,6,5,4, 3,2,1,0 };
   __attribute__((aligned(16)))
   static const uint8_t z[16] =
      { 0,0,0,0,  0,0,0,0,  0,0,0,0,  0,0,0,0 };

   const uint8x16_p8 mask = vec_ld(0, m);
   const uint8x16_p8 zero = vec_ld(0, z);

   uint8x16_p8 data = vec_perm((uint8x16_p8)src, zero, mask);
   vec_st(data, 0, dest);
# else
   vec_vsx_st((uint8x16_p8)src, 0, dest);
# endif
#endif
}

void Print64x2(const uint64x2_p8 block, const char* label)
{
   __attribute__((aligned(16)))
   uint8_t t[16];
   Save64x2(block, t);

   if (label && label[0])
      printf("%s: ", label);

   for (unsigned int i = 0; i<16; ++i) {
      printf("%02X ", t[i]);
      if (i == 7) printf(" ");
   }
   printf("\n");
}

void Print8x16(const uint8x16_p8 block, const char* label)
{
   __attribute__((aligned(16)))
   uint8_t t[16];
   Save8x16(block, t);

   if (label && label[0])
      printf("%s: ", label);

   for (unsigned int i = 0; i<16; ++i) {
      printf("%02X ", t[i]);
      if (i == 7) printf(" ");
   }
   printf("\n");
}

int main(int argc, char* argv[])
{
   /* Initial input, key and a few rounds for testing */
   __attribute__((aligned(16)))
   const uint8_t input[16] = {
      0x32, 0x43, 0xf6, 0xa8, 0x88, 0x5a, 0x30, 0x8d, 0x31, 0x31, 0x98, 0xa2, 0xe0, 0x37, 0x07, 0x34
   };
   __attribute__((aligned(16)))
   const uint8_t key[16] = {
      0x2b, 0x7e, 0x15, 0x16, 0x28, 0xae, 0xd2, 0xa6, 0xab, 0xf7, 0x15, 0x88, 0x9 , 0xcf, 0x4f, 0x3c
   };
   __attribute__((aligned(16)))
   const uint8_t subkeys[10][16] = {
      {0xA0, 0xFA, 0xFE, 0x17, 0x88, 0x54, 0x2c, 0xb1, 0x23, 0xa3, 0x39, 0x39, 0x2a, 0x6c, 0x76, 0x05},
      {0xF2, 0xC2, 0x95, 0xF2, 0x7a, 0x96, 0xb9, 0x43, 0x59, 0x35, 0x80, 0x7a, 0x73, 0x59, 0xf6, 0x7f},
      {0x3D, 0x80, 0x47, 0x7D, 0x47, 0x16, 0xFE, 0x3E, 0x1E, 0x23, 0x7E, 0x44, 0x6D, 0x7A, 0x88, 0x3B},
      {0xEF, 0x44, 0xA5, 0x41, 0xA8, 0x52, 0x5B, 0x7F, 0xB6, 0x71, 0x25, 0x3B, 0xDB, 0x0B, 0xAD, 0x00},
      {0xD4, 0xD1, 0xC6, 0xF8, 0x7C, 0x83, 0x9D, 0x87, 0xCA, 0xF2, 0xB8, 0xBC, 0x11, 0xF9, 0x15, 0xBC},
      {0x6D, 0x88, 0xA3, 0x7A, 0x11, 0x0B, 0x3E, 0xFD, 0xDB, 0xF9, 0x86, 0x41, 0xCA, 0x00, 0x93, 0xFD},
      {0x4E, 0x54, 0xF7, 0x0E, 0x5F, 0x5F, 0xC9, 0xF3, 0x84, 0xA6, 0x4F, 0xB2, 0x4E, 0xA6, 0xDC, 0x4F},
      {0xEA, 0xD2, 0x73, 0x21, 0xB5, 0x8D, 0xBA, 0xD2, 0x31, 0x2B, 0xF5, 0x60, 0x7F, 0x8D, 0x29, 0x2F},
      {0xAC, 0x77, 0x66, 0xF3, 0x19, 0xFA, 0xDC, 0x21, 0x28, 0xD1, 0x29, 0x41, 0x57, 0x5c, 0x00, 0x6E},
      {0xD0, 0x14, 0xF9, 0xA8, 0xC9, 0xEE, 0x25, 0x89, 0xE1, 0x3F, 0x0c, 0xC8, 0xB6, 0x63, 0x0C, 0xA6}
   };

   /* Result */
   __attribute__((aligned(16))) uint8_t result[16];

#if defined(__xlc__) || defined(__xlC__)

   uint8x16_p8 s = Load8x16(input);
   uint8x16_p8 k = Load8x16(key);

   s = vec_xor(s, k);

   Print8x16(s, "State 1");

   k = Load8x16(subkeys[0]);
   s = __vcipher(s, k);

   Print8x16(s, "State 2");

   k = Load8x16(subkeys[1]);
   s = __vcipher(s, k);

   Print8x16(s, "State 3");

   k = Load8x16(subkeys[2]);
   s = __vcipher(s, k);

   Print8x16(s, "State 4");

   k = Load8x16(subkeys[3]);
   s = __vcipher(s, k);

   Print8x16(s, "State 5");

   k = Load8x16(subkeys[4]);
   s = __vcipher(s, k);

   Print8x16(s, "State 6");

   k = Load8x16(subkeys[5]);
   s = __vcipher(s, k);

   Print8x16(s, "State 7");

   k = Load8x16(subkeys[6]);
   s = __vcipher(s, k);

   Print8x16(s, "State 8");

   k = Load8x16(subkeys[7]);
   s = __vcipher(s, k);

   Print8x16(s, "State 9");

   k = Load8x16(subkeys[8]);
   s = __vcipher(s, k);

   Print8x16(s, "State 10");

   k = Load8x16(subkeys[9]);
   s = __vcipherlast(s, k);

   Save8x16(s, result);

#elif defined(__GNUC__) 

   uint64x2_p8 s = Load64x2(input);
   uint64x2_p8 k = Load64x2(key);
   s = vec_xor(s, k);

   k = Load64x2(subkeys[0]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[1]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[2]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[3]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[4]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[5]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[6]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[7]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[8]);
   s = __builtin_crypto_vcipher(s, k);

   k = Load64x2(subkeys[9]);
   s = __builtin_crypto_vcipherlast(s, k);

   Save64x2(s, result);

#endif

   printf("Output: ");
   for (unsigned int i=0; i<16; ++i)
      printf("%02X ", result[i]);
   printf("\n");

   if (result[0] == 0x39 && result[1] == 0x25 && result[2] == 0x84 && result[3] == 0x1D)
      printf("SUCCESS!!!\n");
   else
      printf("FAILURE!!!\n");

   return 0;
}

---

We applied J.H.'s feedback and checked in the source file. However, we left the original question in place so J.H.'s answer did not lose context. The updated file can be found at fips197-p8.c.

Answer 1

Consider deleting this comment:

// #include <builtins.h>

Hmmm.

/* Avoid 0 == 0 when __BYTE_ORDER__ is not defined */
#ifndef __BYTE_ORDER__
# define __BYTE_ORDER__ -16
#endif

Wow. Now I'm really scared, not even sure what universe we're venturing into. Schrödinger's cat is whining and I have no reassuring phrases to whisper.

#if defined(__xlc__) || defined(__xlC__)
/* IBM XL C/C++ compiler */

Ok, second time we've seen this. Maybe it's time to #define a symbol (like __xlc__) based on the disjunction, so we can use a simpler #if in future.

# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
return vec_xl_be(0, s);

This is fascinating. I'm reading it as a suffix of "big endian", and then feeling bad about that, given that clearly we're little endian. I wouldn't mind a comment here.

This line is especially fascinating:

uint8_t* s = (uint8_t*)src;

Now, I understand that you grumbled about it in the narrative, chalking it up to a bug. I would appreciate a brief comment about that in the source, hopefully with an URL or at least bug number. At some point in future, the bug might be closed, so if compiler version is greater than X.Y then crazy s assignment should no longer be needed. It seems like choosing to de-support ancient buggy versions of xlc compiler (in future) would be good for the project.

I'm reading

#if defined(__xlc__) || defined(__xlC__)
   /* IBM XL C/C++ compiler */
# if (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__)
   vec_xst_be(src, 0, dest);
# else
   vec_xst(src, 0, dest);
# endif

These conditionals grow a bit tedious. I propose grouping them in a single preprocessor block, so VEC_XL, VEC_XST, and similar macros are defined en masse, and then each function body is a simple one-liner. In particular, that would let us isolate the s = src bug workaround to a single source line annotated by a single comment that helps us to assess whether the bug has been fixed yet.

I'm reading Load64x2() and Save64x2. It seems like the masking could be factored out into a common macro or common code or something.

In Print64x2, this seems very forgiving:

if (label && label[0])

My perspective is, I told the caller to pass in a label already, and if he didn't, well we're going to segfault and it is entirely the caller's fault. With your defensive programming you're introducing a more complex public API, where there's two ways to decline to pass in an optional parameter, and where testers must present several cases to obtain good code coverage. My preference is for simpler APIs. Same remark for Print8x16.

In main, input, key, & subkeys appear to be drawn from some document, perhaps FIPS 197 (though I didn't notice an occurence there). It would be useful to cite your reference in a comment.

I can't say I'm very happy with the xlc vs. GNUC cases. The amount of code is quite large. It looks very copy-n-pasted. We have an opportunity to insert a for loop. We have an opportunity to #define a symbol that resolves to either Load8x16 or Load64x2, and another the resolves to either __vcipher or __builtin_crypto_vcipher.

This seems very hard coded, which is fine:

if (result[0] == 0x39 && result[1] == 0x25 && result[2] == 0x84 && result[3] == 0x1D)

What I'm looking for is citation within a document, such as FIPS 197.