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

[Jiangli Zhou]

On Tue, 18 Nov 2025 08:53:05 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Jiangli Zhou has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix Whitespace error.
>
> 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.

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

> 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?

Done.

> 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.

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

> 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?

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544352236
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544352547
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544348398
PR Review Comment: https://git.openjdk.org/jdk/pull/28363#discussion_r2544346000