979703 - Implementation of AES in different modes of operation, using AES-NI and PCLMULQDQ-NI, for WIN32 and WIN64 platforms

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Shay Gueron]

Attachment: An NSS patch that implements AES in different modes of operation, using AES-NI and PCLMULQDQ-NI, for WIN32 and WIN64 platforms

Hello everyone, 

This patch implements AES using AES-NI and PCLMULQDQ-NI, for WIN32 and WIN64 platforms (similarly to the Linux64 implementation). The code implements the following AES modes of operation: AES-ECB, AES-CBC, AES-CTR, AES-GCM. 

The performance achieved by this patch, in CPU Cycles/Byte is the following (the numbers in parentheses show the current performance):

Win32        |ECB enc/dec|  CBC enc  |  CBC dec  |CTR enc/dec|GCM enc/dec
-------------------------+-----------+-----------+-----------+------------
Core i5 2520M|0.81(18.42)|5.13(20.52)|0.86(22.05)|0.86(23.89)|3.00(101.90)
-------------------------+-----------+-----------+-----------+------------
Core i7 4770K|0.63(15.24)|4.44(17.39)|0.63(17.68)|0.63(19.11)|1.32(109.97)


Win64        |ECB enc/dec|  CBC enc  |  CBC dec  |CTR enc/dec|GCM enc/dec
-------------------------+-----------+-----------+-----------+------------
Core i5 2520M|0.71(16.48)|5.10(19.09)|0.76(18.33)|0.81(19.71)|2.81(66.29)
-------------------------+-----------+-----------+-----------+------------
Core i7 4770K|0.63(15.04)|4.44(17.39)|0.63(16.64)|0.63(18.02)|1.06(59.21)


Note: This patch does not handle the bug that is patched in https://bugzilla.mozilla.org/show_bug.cgi?id=940794 and https://bugzilla.mozilla.org/show_bug.cgi?id=977829  (i.e., it does not check the OSXSAVE bit, because such a patch already exists, but is not yet part of the main NSS trunk yet). 


Developers and authors:
************************************************************************
Shay Gueron (1, 2), and Vlad Krasnov (1)
(1) Intel Corporation, Israel Development Center, Haifa, Israel
(2) University of Haifa, Israel
************************************************************************
Copyright(c) 2014, Intel Corp.

Comment 1

[Wan-Teh Chang]

Attachment: Patch (for NSS trunk), by Shay Gueron

I applied Shay's patch (attachment 8385838 [details] [diff] [review]) to
the NSS trunk and regenerated this patch. I can
build NSS with the patch for both WIN32 and WIN64.

Comment 2

[Wan-Teh Chang]

Attachment: Patch (for NSS trunk on 2014-04-08), by Shay Gueron

I updated Shay's patch to the current NSS trunk. Now it is
necessary to define the macro INTEL_GCM to enable the Intel
assembly code for GCM.

This patch has bugs in 192-bit and 256-bit AES ECB and CBC
decryption. The next patch fixes these bugs.

Comment 3

[Wan-Teh Chang]

Attachment: Patch v2, by Shay Gueron

Fixed the AES ECB and CBC decryption bugs for 192-bit and 256-bit
keys, in 32-bit builds. The bugs were in the functions
intel_aes_decrypt_init_192 and intel_aes_decrypt_init_256 in
intel-aes-x86-masm.asm. By comparing the functions with the same
functions in intel-aes-x64-masm.asm, it was not hard to spot the
bugs.

Comment 4

[Wan-Teh Chang]

Attachment: Patch v3, by Shay Gueron

Fixed formatting issues.

Comment 5

[Wan-Teh Chang]

Attachment: Patch v4, by Shay Gueron

Fixed more formatting issues, in particular in nss/lib/freebl/Makefile.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/281db105f1f7

I will monitor the NSS build bots tonight. If the Windows build fail,
we just need to back out the change to nss/lib/freebl/Makefile.

Comment 6

[Wan-Teh Chang]

Attachment: Check for Visual C++ 2010 for Intel AES assembly code

Comment 7

[Wan-Teh Chang]

Attachment: Check for Visual C++ 2010 for Intel AES assembly code, v2

64-bit Windows builds also need the same change.

Comment 8

[Kai Engert [:KaiE:]]

(In reply to Wan-Teh Chang from comment #5)
> Patch checked in: https://hg.mozilla.org/projects/nss/rev/281db105f1f7
> 
> I will monitor the NSS build bots tonight. If the Windows build fail,
> we just need to back out the change to nss/lib/freebl/Makefile.


This change failed to build on Windows 2003

hg/nss/lib/freebl/rijndael.c(982) : error C4013: '_xgetbv' undefined; assuming extern returning int

See line 982 here:
https://hg.mozilla.org/projects/nss/file/281db105f1f7/lib/freebl/rijndael.c#l977

Comment 9

[Wan-Teh Chang]

Kai: please review my patch in comment 7. It'll fix the
compilation errors with Visual C++ 2008 and older. Thanks.

Comment 10

[Kai Engert [:KaiE:]]

Comment on attachment 8403771 [details] [diff] [review]
Check for Visual C++ 2010 for Intel AES assembly code, v2

let's try it, I'll check it in as a bustage fix

r=kaie

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 8403771 [details] [diff] [review]
Check for Visual C++ 2010 for Intel AES assembly code, v2

https://hg.mozilla.org/projects/nss/rev/edaddc287517

Comment 12

[Kai Engert [:KaiE:]]

the patch fixed the win 2003 issue

other windows platforms still executing tests, but have succeeded building.

Comment 13

[Wan-Teh Chang]

Attachment: Fix a bug in the gen_aes_cbc_dec_func macro in intel-aes-x64-masm.asm, by Shay Gueron

Shay sent me a patch to fix a bug in loop8 of the gen_aes_cbc_dec_func
macro in intel-aes-x64-masm.asm. Every eighth block of the ciphertext
is incorrect. Our current AES CBC test vectors are too short to catch
this bug. I'm going to add some AES CBC test vectors that have plaintext
and ciphertext longer than two blocks.

This patch also contains a coding style cleanup in nss/lib/freebl/rijndael.c.

Patch checked in:
https://hg.mozilla.org/projects/nss/rev/1f11025a295a
https://hg.mozilla.org/projects/nss/rev/a849a3461539

Comment 14

[Wan-Teh Chang]

Shay's code can also be reviewed at https://codereview.chromium.org/214183004/.
Patch set 1 is roughly the original NSS 3.16 code. So you should review the
delta from patch set 1.

Comment 15

[Wan-Teh Chang]

Comment on attachment 8403710 [details] [diff] [review]
Patch v4, by Shay Gueron

Review of attachment 8403710 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/ctr.c
@@ +172,5 @@
>  }
> +
> +#if defined(USE_HW_AES) && defined(_MSC_VER)
> +SECStatus
> +CTR_Update_HW_AES(CTRContext *ctr, unsigned char *outbuf,

Shay: CTR_Update_HW_AES is the same as CTR_Update except
for the few lines in the middle of the function. It would
be nice to avoid the code duplication. Do you have any good
idea?

Comment 16

[Wan-Teh Chang]

Attachment: Check for Visual C++ 2010 SP1 for the _xgetbv compiler intrinsic function

On Windows, we are calling the _xgetbv compiler intrinsic function, which
was added in Visual C++ 2010 SP1. I had assumed that Mozilla requires
VC 2010 SP1, so I used a simpler compiler version check. But I found that
Mozilla try servers are using Visual C++ 2010 RTM. In this patch I implemented
an exact compiler version check. I also did some cleanups such as using the
Visual C++ internal version 12 for VC 2013 and passing the -nologo option
to the assembler.

Comment 17

[Wan-Teh Chang]

Attachment: Better error handling in intel_AES_GCM_DecryptUpdate, and coding style fixes suggested by Ryan

Ryan: this patch makes the changes you suggested in
https://codereview.chromium.org/214183004/#msg2.

Comment 18

[Wan-Teh Chang]

Attachment: Check for Visual C++ 2010 SP1 for the _xgetbv compiler intrinsic function, v2

Updated the comments to describe exactly what requires VC10 SP1.

Comment 19

[Wan-Teh Chang]

Attachment: Better error handling in intel_AES_GCM_DecryptUpdate, and coding style fixes suggested by Ryan, v2

I found that I can't assert tmp == blocksize after the ctr->cipher
calls because none of the AES ECB encryption worker functions set
the |outputLen| output argument. So I had to remove the three
assertions. I also made two coding style fixes that Adam suggested.

Comment 20

[Ryan Sleevi]

Comment on attachment 8411488 [details] [diff] [review]
Better error handling in intel_AES_GCM_DecryptUpdate, and coding style fixes suggested by Ryan, v2

Review of attachment 8411488 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/intel-gcm-wrap.c
@@ +56,1 @@
>      

It looks like there is a variety of stray whitespace in this file (line 44, line 51, 56, 67, 103, 139, 154, 199, 210) if you want to fix up when patching.

@@ +220,5 @@
> +    if (maxout < inlen) {
> +        *outlen = inlen;
> +        PORT_SetError(SEC_ERROR_OUTPUT_LEN);
> +        return SECFailure;
> +    }

Good catch!

Comment 21

[Wan-Teh Chang]

Comment on attachment 8411473 [details] [diff] [review]
Check for Visual C++ 2010 SP1 for the _xgetbv compiler intrinsic function, v2

Patch checked in: https://hg.mozilla.org/projects/nss/rev/0abdafa757bb

Comment 22

[Wan-Teh Chang]

Attachment: Better error handling in intel_AES_GCM_DecryptUpdate, and coding style fixes suggested by Ryan, v3

Also removed the trailing whitespace in intel-gcm-wrap.c.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/3618ceada7cc

Comment 23

[Wan-Teh Chang]

Attachment: Avoid _xgetbv() for 32-bit x86 builds so that we can use the Intel AES assembly code with VC 2010 RTM

The 64-bit x64 builds still require VC2010 SP1 for the Intel AES
assembly code. Unfortunately inline assembly is only supported
for 32-bit x86, so it is more work to fix 64-bit x64 builds. I'll
deal with that later.

Comment 24

[Wan-Teh Chang]

Attachment: 8412291: Avoid _xgetbv() for 32-bit x86 builds so that we can use the Intel AES assembly code with VC 2010 RTM, v1.1

I removed the opcode for the xgetbv instruction from the
comment. The opcode is not needed to understand the code.

Comment 25

[Camilo Viecco (:cviecco)]

Comment on attachment 8413181 [details] [diff] [review]
8412291: Avoid _xgetbv() for 32-bit x86 builds so that we can use the Intel AES assembly code with VC 2010 RTM, v1.1

Review of attachment 8413181 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/freebl/rijndael.c
@@ +982,5 @@
>      PRUint32 xcr0;
>  #if defined(_MSC_VER)
> +#if defined(_M_IX86)
> +    __asm {
> +        mov ecx, 0

I am assuming ecx is either stored somewhere or there is a guarantee that its contents are OK to be cleared.

Comment 26

[Wan-Teh Chang]

Comment on attachment 8413181 [details] [diff] [review]
8412291: Avoid _xgetbv() for 32-bit x86 builds so that we can use the Intel AES assembly code with VC 2010 RTM, v1.1

Patch checked in: https://hg.mozilla.org/projects/nss/rev/7e8485a5ed49

Comment 27

[Wan-Teh Chang]

Attachment: Disable the Intel AES assembly code on Windows for NSS 3.16.1

Testing of the new assembly code uncovered bugs in intel-gcm-x86-masm.asm.
We should disable the new assembly code on Windows for NSS 3.16.1 and target
NSS 3.16.2.

Comment 28

[Kai Engert [:KaiE:]]

Comment on attachment 8414437 [details] [diff] [review]
Disable the Intel AES assembly code on Windows for NSS 3.16.1

r=kaie

Should this bug be reopened?

Comment 29

[Wan-Teh Chang]

Comment on attachment 8414437 [details] [diff] [review]
Disable the Intel AES assembly code on Windows for NSS 3.16.1

Review of attachment 8414437 [details] [diff] [review]:
-----------------------------------------------------------------

Patch checked in: https://hg.mozilla.org/projects/nss/rev/0bef241d00cb

Comment 30

[Wan-Teh Chang]

Attachment: Change the register from di do dl in intel-gcm-x86-masm.asm, by Shay Gueron

Shay sent me this patch. He said:
  Changed the register from di do dl. I also explicitly added
  the asm directive BYTE PTR, to avoid similar mistake. The
  change is only in intel-gcm-x86-masm.asm. Both enc/dec.

Comment 31

[Wan-Teh Chang]

Attachment: Fix bugs in intel-gcm-x86-masm.asm and re-enable the Intel AES assembly code

Shay's first fix (attachment 8414506 [details] [diff] [review]) is incorrect because it
clobbered the edx register, which is used by the code later.

He and I independently wrote the correct fix. The fix was
reviewed by Adam Langley at
https://codereview.chromium.org/254213002

Patch checked in: https://hg.mozilla.org/projects/nss/rev/f7a4c771997e

Comment 32

[Wan-Teh Chang]

Comment on attachment 8419128 [details] [diff] [review]
Fix bugs in intel-gcm-x86-masm.asm and re-enable the Intel AES assembly code

Patch pushed to mozilla-inbound as part of NSS_3_16_2_BETA1:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4da05df32a63

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4da05df32a63

Comment 34

[Kai Engert [:KaiE:]]

Comment on attachment 8413181 [details] [diff] [review]
8412291: Avoid _xgetbv() for 32-bit x86 builds so that we can use the Intel AES assembly code with VC 2010 RTM, v1.1

clearing sr request, I cannot review it, I'm not an assembler programmer.