Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3527,6 +3527,8 @@ void StubGenerator::aesgcm_avx512(Register in, Register len, Register ct, Regist

__ bind(MESG_BELOW_32_BLKS);
__ subl(len, 16 * 16);
__ cmpl(len, 256);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the stylistic logic, this should be written as 16 * 16, to match the surrounding subl and addl.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed review, @shipilev! Fixed.

__ jcc(Assembler::lessEqual, ENC_DEC_DONE);
__ addl(pos, 16 * 16);
gcm_enc_dec_last_avx512(len, in, pos, AAD_HASHx, SHUF_MASK, avx512_subkeyHtbl, ghashin_offset, HashKey_16, true, true);

Expand Down Expand Up @@ -4016,13 +4018,15 @@ void StubGenerator::aesgcm_avx2(Register in, Register len, Register ct, Register
const Register rounds = r10;
const XMMRegister ctr_blockx = xmm9;
const XMMRegister aad_hashx = xmm8;
Label encrypt_done, encrypt_by_8_new, encrypt_by_8;
Label encrypt_done, encrypt_by_8_new, encrypt_by_8, exit;

//This routine should be called only for message sizes of 128 bytes or more.
//Macro flow:
//process 8 16 byte blocks in initial_num_blocks.
//process 8 16 byte blocks at a time until all are done 'encrypt_by_8_new followed by ghash_last_8'
__ xorl(pos, pos);
__ cmpl(len, 128);
__ jcc(Assembler::less, exit);

//Generate 8 constants for htbl
generateHtbl_8_block_avx2(subkeyHtbl);
Expand Down Expand Up @@ -4090,6 +4094,7 @@ void StubGenerator::aesgcm_avx2(Register in, Register len, Register ct, Register
__ vpxor(xmm0, xmm0, xmm0, Assembler::AVX_128bit);
__ vpxor(xmm13, xmm13, xmm13, Assembler::AVX_128bit);

__ bind(exit);
}

#undef __
113 changes: 113 additions & 0 deletions test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/*
* Copyright (c) 2025, Google LLC. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8371864
* @run main/othervm TestAesGcmIntrinsic
* @summary Test GaloisCounterMode.implGCMCrypt0 AVX512/AVX2 intrinsics.
*/

import java.security.SecureRandom;
import java.security.spec.AlgorithmParameterSpec;
import java.time.Duration;
import javax.crypto.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.GCMParameterSpec;
import javax.crypto.spec.SecretKeySpec;

public class TestAesGcmIntrinsic {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds like TestGCMSplitBound or some such; it is not a generic test for AES/GCM intrinsic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed to TestAesGcmIntrinsic name, when converting the original test into the jtreg test. TestGCMSplitBound SGTM. Changed.


static final SecureRandom SECURE_RANDOM = newDefaultSecureRandom();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by comment: Java code should use 4x whitespace indentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TobiHartmann, thanks! Fixed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need a SecureRandom here? Random RANDOM = Utils.getRandomInstance(); gets you the pre-seeded random instance, which can be used to repeatably reproduce failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept the SecureRandom without changing. I think that could be more related to what the original reproducible.


private static SecureRandom newDefaultSecureRandom() {
SecureRandom retval = new SecureRandom();
retval.nextLong(); // force seeding
return retval;
}

private static byte[] randBytes(int size) {
byte[] rand = new byte[size];
SECURE_RANDOM.nextBytes(rand);
return rand;
}

private static final int IV_SIZE_IN_BYTES = 12;
private static final int TAG_SIZE_IN_BYTES = 16;

private byte[] gcmEncrypt(final byte[] key, final byte[] plaintext, final byte[] aad)
throws Exception {
byte[] nonce = randBytes(IV_SIZE_IN_BYTES);
SecretKey keySpec = new SecretKeySpec(key, "AES");
AlgorithmParameterSpec params =
new GCMParameterSpec(8 * TAG_SIZE_IN_BYTES, nonce, 0, nonce.length);
Cipher cipher = Cipher.getInstance("AES/GCM/NoPadding");
cipher.init(Cipher.ENCRYPT_MODE, keySpec, params);
if (aad != null && aad.length != 0) {
cipher.updateAAD(aad);
}
int outputSize = cipher.getOutputSize(plaintext.length);
int len = IV_SIZE_IN_BYTES + outputSize;
byte[] output = new byte[len];
System.arraycopy(nonce, 0, output, 0, IV_SIZE_IN_BYTES);
cipher.doFinal(plaintext, 0, plaintext.length, output, IV_SIZE_IN_BYTES);
return output;
}

// x86-64 parallel intrinsic data size
private static final int PARALLEL_LEN = 512;
// max data size for x86-64 intrinsic
private static final int SPLIT_LEN = 1048576; // 1MB

private void jitFunc() throws Exception {
byte[] aad = randBytes(20);
byte[] key = randBytes(16);
// Force JIT.
for (int i = 0; i < 100000; i++) {
byte[] message = randBytes(PARALLEL_LEN);
byte[] ciphertext = gcmEncrypt(key, message, aad);
if (ciphertext == null) {
throw new RuntimeException("ciphertext is null");
}
}
for (int messageSize = SPLIT_LEN; messageSize < SPLIT_LEN + 300; messageSize++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SPLIT_LEN - 300; SPLIT_LEN + 300] for completeness, perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

byte[] message = randBytes(messageSize);
try {
byte[] ciphertext = gcmEncrypt(key, message, aad);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it makes sense to check that round-trip is successful, e.g. that decrypt(encrypt(message)) == message. Currently we implicitly rely on exceptions being thrown from the incorrectly executing code, which is IMO too weak -- in the boundary conditions like these, there might be bugs that do not manifest in visible exceptions, and just the encryption is subtly broken.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea. I added decrypt part and the check as suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the changes, there were more common parts in the test. I moved common code into helper methods.

if (ciphertext == null) {
throw new RuntimeException("ciphertext is null");
}
} catch (Exception e) {
throw new Exception("Failed for messageSize " + Integer.toHexString(messageSize), e);
}
}
}

public static void main(String[] args) throws Exception {
TestAesGcmIntrinsic test = new TestAesGcmIntrinsic();
long startTime = System.currentTimeMillis();
while (System.currentTimeMillis() - startTime < 60 * 1000) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get that you want a stress test. But time-limiting puts the test into weird condition: it can have different number of iterations, depending on auxiliary load on the machine. These tests are running in parallel with lots of other tests, so it is not uncommon. Do you even need to repeat jitFunc() call multiple times? Looks like it traverses the interesting configurations in one go?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing today. For 200 runs, removing the time-limited loop, there is 89 runs out of 200 fail. So I changed to use an iteration of three runs, all 200 runs fail without the fix.

test.jitFunc();
}
}
}