Implement AES in different modes of operation, using AES-NI and

[Replaced by https://codereview.chromium.org/250463004/ for the commit queue.]

Implement AES in different modes of operation, using AES-NI and
PCLMULQDQ-NI, for WIN32 and WIN64 platforms. Only the WIN32 assembly
code is used right now.

By Shay Gueron and Vlad Krasnov, Intel.
Upstream NSS bug: https://bugzilla.mozilla.org/show_bug.cgi?id=979703

R=agl@chromium.org,rsleevi@chromium.org
BUG=none

[wtc, 2014-04-23 18:13:19]

This CL is now ready for review. I had to enhance GYP to assemble assembly code
with the /safeseh option.

In patch set 1 I added three existing NSS files that Chromium didn't need
before. So the actual changes are the delta from patch set 1.

[Ryan Sleevi, 2014-04-23 19:53:34]

I have not yet reviewed the ASM - that may take some time for me to mentally map
in MASM again, although if Adam is happy with it, I'm fine to defer that part to
him.

The patch still seems to use a variety of styles that I think we should fix.
Since your patchset revisions seem to indicate you're fixing style issues in
landing this, I've tried to include such feedback here.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c
File nss/lib/freebl/ctr.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:179: unsigned int blocksize)
Alignment?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:192: ctr_xor(outbuf, inbuf, ctr->buffer+ctr->bufPtr,
needed);
spaces here between ctr->buffer and ctr->bufPtr?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:209: inlen &= 16 - 1;
This style surprises me, if only because it seems to subtly rely on integer
promotion rules. Is there a less surprising way to express this?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:214: rv = (*ctr->cipher)(ctr->context, ctr->buffer, &tmp,
blocksize,
Should we add a PORT_Assert that tmp == blocksize?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:216: ctr_GetNextCtr(ctr->counter, ctr->counterBits,
blocksize);
Why does this happen before the rv check?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
File nss/lib/freebl/intel-gcm-wrap.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:52: int IV_whole_len =
gcmParams->ulIvLen&(~0xf);
type conversion warning - ulIvLen is ulong, but this is an int. I can't help but
feel nervous about this.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:55: int AAD_remainder_len =
gcmParams->ulAADLen&0xf;
spaces between operators for readability?

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:81: /* Init the counter */
line breaks for readability between these logical steps? (line 71, 76, 78, 81,
87)

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:83: _mm_storeu_si128((__m128i*)gcm->CTR,
_mm_setr_epi32(((unsigned int*)gcmParams->pIv)[0], ((unsigned
int*)gcmParams->pIv)[1], ((unsigned int*)gcmParams->pIv)[2], 0x01000000));
Undefined aliasing behaviour (the "unsigned int*" derefing into the byte array)

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:95: (
NSS style (eg: libpkix) has tended to keep the opening paren on the end of the
line (line 94)

This wrapping is inconsistent with the wrapping on line 83)

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:102: );
indent off by one from lines 92 through 102

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:141: unsigned char *outbuf,
indent style

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:175: T);
Inconsistent wrapping behaviour here and on line 161 when compared with lines 94
and 83/112

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:180: {
inconsistent brace style compared with lines 155, 151

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:187: unsigned char *outbuf,
indent

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:225: return SECFailure;
You should set *outlen to zero here - you still leave the decrypted data in
outbuf, so a misbehaving application (that doesn't check the return value) could
end up using the decrypted data even though the tag check failed, and things
would Just Work

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/rijndael.c
File nss/lib/freebl/rijndael.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/rijndael...
nss/lib/freebl/rijndael.c:1179: cx->worker = (freeblCipherFunc) CTR_Update;
Seems better to use explicit braces in this conditional to avoid any goto fail
shenanigans

[wtc, 2014-04-24 01:04:09]

Ryan: thanks for the review. In patch set 10, I made the changes you suggested.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c
File nss/lib/freebl/ctr.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:179: unsigned int blocksize)

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Alignment?

Done. I will also align the parameters of the other functions in this file in
the NSS upstream patch.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:192: ctr_xor(outbuf, inbuf, ctr->buffer+ctr->bufPtr,
needed);
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> spaces here between ctr->buffer and ctr->bufPtr?

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:209: inlen &= 16 - 1;

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> This style surprises me, if only because it seems to subtly rely on integer
> promotion rules. Is there a less surprising way to express this?

Done. This code also assumes |blocksize| is 16. I copied similar code from
nss/lib/freebl/cts.c to fix this.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:214: rv = (*ctr->cipher)(ctr->context, ctr->buffer, &tmp,
blocksize,
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Should we add a PORT_Assert that tmp == blocksize?

Done.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/ctr.c#ne...
nss/lib/freebl/ctr.c:216: ctr_GetNextCtr(ctr->counter, ctr->counterBits,
blocksize);

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Why does this happen before the rv check?

I don't really know. Perhaps the author wanted to make sure the counter value
isn't reused even after an encryption failure.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
File nss/lib/freebl/intel-gcm-wrap.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:52: int IV_whole_len =
gcmParams->ulIvLen&(~0xf);
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> type conversion warning - ulIvLen is ulong, but this is an int. I can't help
but
> feel nervous about this.

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:55: int AAD_remainder_len =
gcmParams->ulAADLen&0xf;

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> spaces between operators for readability?

I will fix these in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:81: /* Init the counter */
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> line breaks for readability between these logical steps? (line 71, 76, 78, 81,
> 87)

I will fix these in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:83: _mm_storeu_si128((__m128i*)gcm->CTR,
_mm_setr_epi32(((unsigned int*)gcmParams->pIv)[0], ((unsigned
int*)gcmParams->pIv)[1], ((unsigned int*)gcmParams->pIv)[2], 0x01000000));

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Undefined aliasing behaviour (the "unsigned int*" derefing into the byte
array)

Acknowledged. Any suggestion on how to fix this? I guess I can use three
memcpy() calls to convert the bytes to ints.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:95: (
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> NSS style (eg: libpkix) has tended to keep the opening paren on the end of the
> line (line 94)
> 
> This wrapping is inconsistent with the wrapping on line 83)

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:102: );

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> indent off by one from lines 92 through 102

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:141: unsigned char *outbuf,
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> indent style

Done.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:175: T);

On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Inconsistent wrapping behaviour here and on line 161 when compared with lines
94
> and 83/112

In the NSS upstream, I will change line 94 to match the style used here. I will
wrap line 83 in a better way, but leave line 112 alone. I don't know how to wrap
line 112 in a readable way :-)

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:180: {
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> inconsistent brace style compared with lines 155, 151

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:187: unsigned char *outbuf,
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> indent

Done.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:225: return SECFailure;
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> You should set *outlen to zero here - you still leave the decrypted data in
> outbuf, so a misbehaving application (that doesn't check the return value)
could
> end up using the decrypted data even though the tag check failed, and things
> would Just Work

Done.

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/rijndael.c
File nss/lib/freebl/rijndael.c (right):

https://codereview.chromium.org/214183004/diff/140001/nss/lib/freebl/rijndael...
nss/lib/freebl/rijndael.c:1179: cx->worker = (freeblCipherFunc) CTR_Update;
On 2014/04/23 19:53:34, Ryan Sleevi wrote:
> Seems better to use explicit braces in this conditional to avoid any goto fail
> shenanigans

Done.

[agl, 2014-04-24 01:30:37]

LGTM.

(I'll have to assume that the KATs will test the ASM code. I've not traced it
carefully.)

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
File nss/lib/freebl/intel-gcm-wrap.c (right):

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:78: /* Initial TAG value is zero*/
space after zero.

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:180: {
odd brace placement for the file.

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gcm.h
File nss/lib/freebl/intel-gcm.h (right):

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm.h:34: SECStatus
intel_AES_GCM_EncryptUpdate(intel_AES_GCMContext  *gcm, unsigned char *outbuf,
two spaces after "intel_AES_GCMContext"

[wtc, 2014-04-24 17:45:41]

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
File nss/lib/freebl/intel-gcm-wrap.c (right):

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:78: /* Initial TAG value is zero*/
On 2014/04/24 01:30:38, agl wrote:
> space after zero.

I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm-wrap.c:180: {
On 2014/04/24 01:30:38, agl wrote:
> odd brace placement for the file.

Ryan also noted this. I will fix this in the NSS upstream.

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gcm.h
File nss/lib/freebl/intel-gcm.h (right):

https://codereview.chromium.org/214183004/diff/160001/nss/lib/freebl/intel-gc...
nss/lib/freebl/intel-gcm.h:34: SECStatus
intel_AES_GCM_EncryptUpdate(intel_AES_GCMContext  *gcm, unsigned char *outbuf,
On 2014/04/24 01:30:38, agl wrote:
> two spaces after "intel_AES_GCMContext"

I will fix this in the NSS upstream.

[wtc, 2014-04-24 17:47:16]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-04-24 17:50:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 17:50:06]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.