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

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

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

[wtc, 2014-04-30 02:00:50]

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
File nss/lib/freebl/intel-gcm-x86-masm.asm (left):

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:847: mov [esp + KS], di

Here we want to copy one byte at a time. Because of a typo, the 16-bit di
register was used, so we ended up copying two bytes at a time. This bug caused
the read access violation crashes. The fix is to use an 8-bit register such as
dl.

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:1192: jmp @b

This block of code seems to be a copy and paste error. It overwrites the
plaintext buffer PT.

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
File nss/lib/freebl/intel-gcm-x86-masm.asm (right):

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:842: mov edi, edx

The code below uses the 8-bit dl register (which is part of the edx register).
edx is used for both Htbl and Gctx, so its value needs to be saved. After this
point, the edi register is not used, so I save the value of edx in edi. Is there
a better way to save and restore the value of a register?

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:869: mov edx, edi

Here we restore the value of edx because we will use Htbl shortly (on line 875).

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:1171: mov edx, edi

Here we restore the value of edx because we will use Htbl shortly (on line
1176).

[agl, 2014-04-30 02:03:26]

I'll have to review this in the morning I'm afraid! I don't think I can do
justice to an asm review this late.

[agl, 2014-04-30 17:38:10]

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
File nss/lib/freebl/intel-gcm-x86-masm.asm (left):

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:847: mov [esp + KS], di
On 2014/04/30 02:00:50, wtc wrote:
> 
> Here we want to copy one byte at a time. Because of a typo, the 16-bit di
> register was used, so we ended up copying two bytes at a time. This bug caused
> the read access violation crashes. The fix is to use an 8-bit register such as
> dl.

I think this can be fixed with just:

mov di, BYTE PTR [PT + KS]

that would avoid more register shuffling. I don't know MASM and it would be
worth looking at the disassembly of the generated object file, but that should
do. If not, it might need movzx rather than mov. (movzx = MOVe with Zero eXtend)

[wtc, 2014-04-30 21:27:21]

Shay sent me his patch for the bugs. His patch is essentially the same as my
patch. The main difference is that he used the symbolic alias 'aluTMP' instead
of 'edi'. Patch set 3 is his patch.

I prefer using the register name 'edi' directly when we are not using it as
"aluTMP".

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
File nss/lib/freebl/intel-gcm-x86-masm.asm (left):

https://codereview.chromium.org/254213002/diff/1/nss/lib/freebl/intel-gcm-x86...
nss/lib/freebl/intel-gcm-x86-masm.asm:847: mov [esp + KS], di

Thanks for the suggestion.  MASM doesn't allow

    mov di, BYTE PTR [PT + KS]

but allows

    movzx di, BYTE PTR [PT + KS]

However, it seems that I can't move the di register to a one-byte memory
location. I'm afraid that we'll need to use an 8-bit register and deal with the
register shuffling.

[agl, 2014-04-30 21:30:44]

LGTM

https://codereview.chromium.org/254213002/diff/20001/nss/lib/freebl/intel-gcm...
File nss/lib/freebl/intel-gcm-x86-masm.asm (left):

https://codereview.chromium.org/254213002/diff/20001/nss/lib/freebl/intel-gcm...
nss/lib/freebl/intel-gcm-x86-masm.asm:1188: cmp KS, 16
Just highlighting that this version of the change also removes this zero-filling
to the next 16 byte granularity.

[wtc, 2014-04-30 21:36:58]

Thanks for the review.

Do you prefer 'aluTMP' or 'edi' when we shuffle registers?

https://codereview.chromium.org/254213002/diff/20001/nss/lib/freebl/intel-gcm...
File nss/lib/freebl/intel-gcm-x86-masm.asm (left):

https://codereview.chromium.org/254213002/diff/20001/nss/lib/freebl/intel-gcm...
nss/lib/freebl/intel-gcm-x86-masm.asm:1188: cmp KS, 16

This removal is also in the previous version of the CL. This loop writes 0 bytes
beyond the end of the PT buffer, corrupting memory.

[agl, 2014-04-30 21:41:17]

On 2014/04/30 21:36:58, wtc wrote:
> Do you prefer 'aluTMP' or 'edi' when we shuffle registers?

No preference.
 
> This removal is also in the previous version of the CL. This loop writes 0
bytes
> beyond the end of the PT buffer, corrupting memory.

Ah, sorry. I stopped when I erroneously thought that the loop could be fixed
without shuffling registers.

[wtc, 2014-04-30 22:43:27]

The CQ bit was checked by wtc@chromium.org

[commit-bot: I haz the power, 2014-04-30 22:43:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wtc@chromium.org/254213002/40001

[commit-bot: I haz the power, 2014-04-30 22:44:49]

Change committed as 267366