1400603 - Reorganize AES-GCM source code based on hw/sw implementation

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Daiki Ueno [:ueno]]

Attachment: 0001-freebl-Make-it-possible-to-disable-SSE2-at-compile-t.patch

Since this commit:
https://hg.mozilla.org/projects/nss/rev/cd068f7ce6ae11120f8e4427aa2e8ac35a69e764
NSS build enables the GCC options -mpclmul and -maes.

Although the corresponding CPU features (CLMUL, AES-NI, AVX) can be turned off at runtime through envvars, the compiler options also imply optimization using
SSE2, which is not yet been mandated in Fedora 27 i686:
$ gcc -dM -E -m32 -mpclmul -maes - < /dev/null | grep SSE2
#define __SSE2__ 1

I tried to disable it by adding -mno-sse2, but then NSS build fails in compiling gcm.c and rijndael.c:
cc -o Linux4.11_x86_cc_glibc_PTH_DBG.OBJ/Linux_SINGLE_SHLIB/gcm.o -c -g -fPIC -Di386 -m32 -pipe -ffunction-sections -fdata-sections -DHAVE_STRERROR -DLINUX -Dlinux -Wall -Werror -DXP_UNIX -DSHLIB_SUFFIX=\"so\" -DSHLIB_PREFIX=\"lib\" -DSHLIB_VERSION=\"3\" -DSOFTOKEN_SHLIB_VERSION=\"3\" -DRIJNDAEL_INCLUDE_TABLES -DDEBUG -UNDEBUG -DDEBUG_ueno -D_REENTRANT -DNSS_NO_INIT_SUPPORT -DUSE_UTIL_DIRECTLY -DNO_NSPR_10_SUPPORT -DSSL_DISABLE_DEPRECATED_CIPHER_SUITE_NAMES -DFREEBL_NO_DEPEND -DFREEBL_LOWHASH -DNSS_X86_OR_X64 -DNSS_X86 -DMP_ASSEMBLY_MULTIPLY -DMP_ASSEMBLY_SQUARE  -DMP_ASSEMBLY_DIV_2DX1D -DMP_USE_UINT_DIGIT -DMP_IS_LITTLE_ENDIAN -DMP_API_COMPATIBLE -I../../../dist/Linux4.11_x86_cc_glibc_PTH_DBG.OBJ/include -I../../../dist/public/nss -I../../../dist/private/nss -Impi -Iecl -Iverified  -std=gnu99 -mpclmul -maes -mno-sse2 gcm.c
In file included from gcm.h:12:0,
                 from gcm.c:12:
gcm.c: In function ‘gcm_HashMult_hw’:
/usr/lib/gcc/x86_64-redhat-linux/7/include/emmintrin.h:1290:1: error: inlining failed in call to always_inline ‘_mm_xor_si128’: target specific option mismatch
 _mm_xor_si128 (__m128i __A, __m128i __B)

The attached patch makes it possible to disable those CPU features at build time, by exposing the envvars as make variables.  I'm not happy with it because it's a bit too intrusive, but it seems to work:
https://bugzilla.redhat.com/show_bug.cgi?id=1482798

Comment 1

[Franziskus Kiefer [:franziskus]]

I'd r- this patch. If you really want to fix this, I'd suggest creating a separate static library that holds the code that requires pclmul and aes-ni and set the flags only for that lib.

Comment 2

[Daiki Ueno [:ueno]]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #1)
> I'd r- this patch. If you really want to fix this, I'd suggest creating a
> separate static library that holds the code that requires pclmul and aes-ni
> and set the flags only for that lib.

My first attempt was something like that:
http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-disable-sse2.patch?h=f27
However, it didn't work, because GCC tries to optimize other portion of code in gcm.c and rijndael.c with SSE2.

It might work if we completely separate hw/sw implementations in different compilation units and define the separate entry points for these implementations.

Comment 3

[Daiki Ueno [:ueno]]

(In reply to Daiki Ueno [:ueno] from comment #2)

> http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-
> disable-sse2.patch?h=f27

Sorry, I meant this one:
http://pkgs.fedoraproject.org/cgit/rpms/nss-softokn.git/tree/nss-softokn-disable-sse2.patch?h=f27&id=4d18ff5c60cc299f0218ceccadf0d076461cfea3

> It might work if we completely separate hw/sw implementations in different
> compilation units and define the separate entry points for these
> implementations.

Will try this.

Comment 4

[Daiki Ueno [:ueno]]

Attachment: 0001-freebl-Reorganize-AES-GCM-source-code-based-on-hw-sw.patch

This patch splits the x86 hardware implementation of AES and GCM to separate compilation units so they can be compiled with specific compiler options (-mpclmul and -maes).

After compiling it with make, the sw-only implementation doesn't seem to contain any SSE2 instructions:
$ objdump -D gcm.o rijndael.o | grep xmm
 3ed:	0f 11 1e             	movups %xmm3,(%esi)
note, movups is an SSE instruction

$ objdump -D gcm-hw.o rijndael-hw.o | grep xmm
...
 2ea:   66 0f 73 f8 08          pslldq $0x8,%xmm0
...
pssldq is an SSE2 instruction: http://www.felixcloutier.com/x86/PSLLDQ.html

Comment 5

[Franziskus Kiefer [:franziskus]]

We use phabricator nowadays for NSS patches [1]. I'm fine with small patches in splinter but bigger ones like this one should definitely go to phabricator. You can link it to your bugzilla account. So this should be a quick thing.

[1] https://phabricator.services.mozilla.com/

Comment 6

[Daiki Ueno [:ueno]]

Thanks for the information, I have submitted it as https://phabricator.services.mozilla.com/D65

Comment 7

[Robert Relyea]

This still isn't fixed:

Add Multi-Factor Authentication To Your Account

    Before you can use Phabricator, you need to add multi-factor authentication to your account.
    Multi-factor authentication helps secure your account by making it more difficult for attackers to gain access or take sensitive actions.
    To learn more about multi-factor authentication, click the Help button below.
    To add an authentication factor, click the Add Authentication Factor button below.
    To continue, add at least one authentication factor to your account.

Comment 8

[Phabricator Automation]

Comment on attachment 8909264 [details]
freebl: Reorganize AES-GCM source code based on hw/sw implementation

Franziskus Kiefer [:fkiefer or :franziskus] has approved the revision.

https://phabricator.services.mozilla.com/D65#1524

Comment 9

[Franziskus Kiefer [:franziskus]]

landed D65 as https://hg.mozilla.org/projects/nss/rev/e84403331d99bb1fcad4a879f42749332861e8e1

Comment 10

[Eric Rescorla (:ekr)]

Right now there's some glitchiness in the Phabricator integration where it requires separate 2FA but at the end of the day I think you're going to need it anyway because Phab is going to depend on bugzilla and bugzilla.mozilla.org is rapidly moving towards requiring 2FA:

https://groups.google.com/forum/#!searchin/mozilla.dev.platform/2fa%7Csort:relevance/mozilla.dev.platform/eemHHhf3lBM/k9cyMceyAQAJ

I'm not sure if this applies to you now (do you have sec bugs access on NSS? I don't) but I imagine they're going to start imposing it on just about everyone who has any sec bugs access soon enough. You may want to complain on dev.platform


(In reply to Robert Relyea from comment #7)
> This still isn't fixed:
> 
> Add Multi-Factor Authentication To Your Account
> 
>     Before you can use Phabricator, you need to add multi-factor
> authentication to your account.
>     Multi-factor authentication helps secure your account by making it more
> difficult for attackers to gain access or take sensitive actions.
>     To learn more about multi-factor authentication, click the Help button
> below.
>     To add an authentication factor, click the Add Authentication Factor
> button below.
>     To continue, add at least one authentication factor to your account.

Comment 11

[Franziskus Kiefer [:franziskus]]

Daiki, do you still need a way to disable this at compile time or is the patch that landed enough for the issues you're seeing?

Comment 12

[Daiki Ueno [:ueno]]

Yes, the landed patch is sufficient; in my opinion whether enabling/disabling SSE2 is a downstream issue.

Comment 13

[Franziskus Kiefer [:franziskus]]

Ok, I changed the title to reflect what was actually done here.
If there're still issues, please open a new bug.