RFR: 8371864: GaloisCounterMode.implGCMCrypt0 AVX512/AVX2 intrinsics stubs cause AES-GCM encryption failure for certain payload sizes

[Aleksey Shipilev]

On Mon, 17 Nov 2025 22:34:14 GMT, Jiangli Zhou <jiangli at openjdk.org> wrote:

> Please review the fix in StubGenerator::aesgcm_avx512 and StubGenerator::aesgcm_avx2 to handle some edge cases with input sizes that are not multiple of the block size. 
> 
> Thanks to Thomas Holenstein and Lukas Zobernig for analyzing the issue and providing the test case!

Good catch! Some stylistic comments for the product fix, and suggestions for the test.

src/hotspot/cpu/x86/stubGenerator_x86_64_aes.cpp line 3530:

> 3528:   __ bind(MESG_BELOW_32_BLKS);
> 3529:   __ subl(len, 16 * 16);
> 3530:   __ cmpl(len, 256);

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

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 39:

> 37: import javax.crypto.spec.SecretKeySpec;
> 38: 
> 39: public class TestAesGcmIntrinsic {

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

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 41:

> 39: public class TestAesGcmIntrinsic {
> 40: 
> 41:   static final SecureRandom SECURE_RANDOM = newDefaultSecureRandom();

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.

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 93:

> 91:       }
> 92:     }
> 93:     for (int messageSize = SPLIT_LEN; messageSize < SPLIT_LEN + 300; messageSize++) {

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

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 96:

> 94:       byte[] message = randBytes(messageSize);
> 95:       try {
> 96:         byte[] ciphertext = gcmEncrypt(key, message, aad);

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.

test/jdk/com/sun/crypto/provider/Cipher/AES/TestAesGcmIntrinsic.java line 109:

> 107:     TestAesGcmIntrinsic test = new TestAesGcmIntrinsic();
> 108:     long startTime = System.currentTimeMillis();
> 109:     while (System.currentTimeMillis() - startTime < 60 * 1000) {

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?

-------------

PR Review: https://git.openjdk.org/jdk/pull/28363#pullrequestreview-3476170085
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536853113
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536930778
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536948179
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536932434
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536921000
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2536910947