AES-NI troubles on MS Windows (gcc compiler)

[karel-m]

64-bit MS Windows, perl 5.32, compiler: gcc 8.3.0 (= strawberry-perl-5.32.1.1-64bit-portable.zip)

Running t/cipher_aes.t crashes during Crypt::Cipher::AES->new(..) which turns out to be a crash in aesni_setup

Specifically here:

   for (i = 1; i < skey->rijndael.Nr; i++) {
      rrk -= 4;
      rk += 4;
      temp = temp_invert(rk);
      *((__m128i*) rk) = temp_invert(rrk);         <<<<<<<<<<<<<< HERE
   }

Not seen this kind of crash on linux.

@sjaeckel any idea?

[sjaeckel]

Nope, haven't seen that before, but also didn't try out AES-NI on Windows. I'll also have a look tomorrow.

[karel-m]

I suspect memory (non)alignment of symmetric_key structure.

1/ I have included symmetric_key into cipher_struct like this:

typedef struct cipher_struct {
  symmetric_key skey;
  struct ltc_cipher_descriptor *desc;
} *Crypt__Cipher;

2/ For dynamic memory allocation I use:

Newz(0, RETVAL, 1, struct cipher_struct);

which is some old perl macro/wrapper around malloc

[karel-m]

Yes, it is an alignment issue:

• on Linux/Mac/*BSD the malloc() seems to allocate memory aligned to 16
• on MS Windows malloc() allocates memory aligned to 8

A dirty hack is to use _aligned_malloc(..) on MS Windows, not sure how to handle it properly in perl/XS code.

Or should we check *skey pointer for alignment here?

int AES_SETUP(const unsigned char *key, int keylen, int num_rounds, symmetric_key *skey)
{
#ifdef LTC_HAS_AES_NI
   if (s_aesni_is_supported()) {
      return aesni_setup(key, keylen, num_rounds, skey);
   }
#endif
   /* Last resort, software AES */
   return rijndael_setup(key, keylen, num_rounds, skey);
}

as passing unaligned skey to aesni_setup(..) means inevitable crash.

It might be also nice to have something like LTC_NO_AES_NI in libtomcrypt so that one can forcefully disable the whole AES-NI stuff.

[karel-m]

Another potential source of troubles are libtomcrypt structures like:

typedef struct {
   int             cipher_idx,
                   buflen,
                   blklen;
   unsigned char   block[MAXBLOCKSIZE],
                   prev[MAXBLOCKSIZE],
                   Lu[2][MAXBLOCKSIZE];
   symmetric_key   key;
} omac_state;

where symmetric_key member is preceded by couple of other members which make a little bit unclear what the final memory alignment of symmetric_key will be. IMO we should move symmetric_key key at the beginning to minimize alignment-related surprises.

[sjaeckel]

Can you check whether this fixes the issue?

I'm not sure whether there's a better way than doing this in all places that require alignment of struct members...

[karel-m]

Yes, your patch works. I am for merging the patch to libtomcrypt as it is much more robust. Perhaps only add some comment why the code is as it is.

[karel-m]

@sjaeckel UPDATE:

This compiler gcc version 13.1.0 (MinGW-W64 x86_64-msvcrt-posix-seh, built by Brecht Sanders) is not quite happy with the patch:

ltc/ciphers/aes/aes.c: In function 'rijndael_setup':
ltc/ciphers/aes/aes.c:116:17: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  116 |     K = (void*)((unsigned long)&skey->rijndael.K[15] & (~0xFuL));
      |                 ^
ltc/ciphers/aes/aes.c:116:9: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
  116 |     K = (void*)((unsigned long)&skey->rijndael.K[15] & (~0xFuL));
      |         ^

ltc/ciphers/aes/aesni.c: In function 'aesni_setup':
ltc/ciphers/aes/aesni.c:64:16: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
   64 |    K = (void*)((unsigned long)&skey->rijndael.K[15] & (~0xFuL));
      |                ^
ltc/ciphers/aes/aesni.c:64:8: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
   64 |    K = (void*)((unsigned long)&skey->rijndael.K[15] & (~0xFuL));
      |        ^

and the test suite fails the same way as when the alignment is wrong.

You can download the gcc in question in strawberry-perl-5.38.0.1-64bit-portable.zip from https://github.com/StrawberryPerl/Perl-Dist-Strawberry/releases

[karel-m]

the gcc where the patch did work was gcc version 8.3.0 (x86_64-posix-seh, Built by strawberryperl.com project)

[sjaeckel]

Hmm, I built with gcc 13.2.1 and clang 16.0.6 and both are happy with the code as committed ...

[karel-m]

The warning happens with gcc-13.1.0 only when I use -msse4.1 -maes compiler options.

[sjaeckel]

I'll have another look later!

[karel-m]

This seems to work:

K = (void*)((((ulong64)&skey->rijndael.K[15]) >> 4) << 4);

but the ulong64 part is ugly.

[karel-m]

Perhaps:

K = (void*)((((size_t)&skey->rijndael.K[15]) >> 4) << 4);

[sjaeckel]

Hmm, I used unsigned long because I forgot how the 64bit data model exactly works and didn't want to introduce uintptr_t ... but I guess we'll have to go that way. size_t would also be an option, but uintptr_t is the better one IMO. OK for you as well?

[sjaeckel]

Perhaps:

K = (void*)((((size_t)&skey->rijndael.K[15]) >> 4) << 4);

My guess is that this will work very often, but in the end it's most likely not such a good idea, since we'd also zero-out the upper 4 bits like that ;)

[karel-m]

Well, uintptr_t might be better but it means "poluting" libtomcrypt with stdint.h and you know my attitude to stdint_h :)

[karel-m]

My guess is that this will work very often, but in the end it's most likely not such a good idea, since we'd also zero-out the upper 4 bits like that ;)

Are you sure?

#include <stdio.h>

int main (void) {
    size_t N;
    void *K;
    N = 18446744073709551615ULL;    // 0xffffffffffffffff
    K = (void*)N;
    printf("ptr1=%p\n", K);         // ptr1=ffffffffffffffff
    K = (void*)((((size_t)K) >> 4) << 4);
    printf("ptr2=%p\n", K);         // ptr2=fffffffffffffff0
    return 0;
}

[sjaeckel]

omfg ... no, what I wrote was wrong.

... I really shouldn't write comments while doing other stuff :smile:

[sjaeckel]

Well, uintptr_t might be better but it means "poluting" libtomcrypt with stdint.h and you know my attitude to stdint_h :)

TBH I'm not so sure whether I'd prefer to pollute the codebase by using the wrong type or "pollute" it by adding the dependency to a header file that originates from a standard which is legally allowed to drink even in the US...

... but maybe that opinion is also wrong...

[karel-m]

I understand, but here in the "perl world" sooner or later a guy with some esoteric IBM, HP or other legacy stuff will create an issue that my module does not compile on his platform.

[sjaeckel]

Fine like that?

[karel-m]

solved by the latest libtomcrypt:develop