On Wed, 15 Oct 2025 23:04:10 GMT, Shawn M Emery <[email protected]> wrote:
>> General:
>> -----------
>> i) This work is to replace the existing AES cipher under the Cryptix license.
>>
>> ii) The lookup tables are employed for performance, but also for operating
>> in constant time.
>>
>> iii) Several loops have been unrolled for optimization purposes, but are
>> harder to read and don't meet coding style guidelines.
>>
>> iv) None of the AES related intrinsics has been modified in this PR, but the
>> new code has been updated to use the intrinsics related hooks for the AES
>> block and key table arguments.
>>
>> Note: I've purposefully not seen the original Cryptix code, so when making a
>> code review comment please don't quote the code in the AESCrypt.java file.
>>
>> Correctness:
>> -----------------
>> The following AES-specific regression tests have passed in intrinsics
>> (default) and non-intrinsic (-Xint) modes:
>>
>> i) test/jdk/com/sun/crypto/provider/Cipher/AES: all 27 tests pass
>>
>> -intrinsics mode for:
>>
>> ii) test/hotspot/jtreg/compiler/codegen/aes: all 4 tests pass
>>
>> iii) jck:api/java_security, jck:api/javax_crypto, jck:api/javax_net,
>> jck:api/javax_security, jck:api/org_ietf, and jck:api/javax_xml/crypto:
>> passed, with 10 known failures
>>
>> iv) jdk_security_infra: passed, with 48 known failures
>>
>> v) tier1 and tier2: all 110257 tests pass
>>
>> Security:
>> -----------
>> In order to prevent side-channel (timing and differential power analysis)
>> attacks the code has been constructed to operate in constant time and does
>> not use conditionals based on the key or key expansion table. This is
>> accomplished by using lookup tables in both the cipher and inverse cipher of
>> AES.
>>
>> Performance:
>> ------------------
>> All AES related benchmarks have been executed against the new and original
>> Cryptix code:
>>
>> micro:org.openjdk.bench.javax.crypto.AES
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESBench
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESExtraBench
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESGCMBench
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESGCMByteBuffer
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESGCMCipherInputStream
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESGCMCipherOutputStream
>>
>> micro:org.openjdk.bench.javax.crypto.full.AESKeyWrapBench.
>>
>> micro:org.openjdk.bench.java.security.CipherSuiteBench (AES)
>>
>> The benchmarks were executed in different compiler modes (default (no
>> compiler options), -Xint, and -Xcomp) and on two different architectures
>> (x86 and arm64) with the following encryption re...
>
> Shawn M Emery has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updates for code review comments from @valeriepeng
src/java.base/share/classes/com/sun/crypto/provider/AES_Crypt.java line 896:
> 894: if (key.length == AESConstants.AES_KEYSIZES[0]) {
> 895: rounds = AES_128_ROUNDS;
> 896: nk = AESConstants.AES_KEYSIZES[0]/WB;
Looks like we can get rid of `nk` as the `genRKeys(byte[])` method can
calculate/derive it based on the specified `key` argument, i.e. `key.length >>
2` or `key.length / WB`
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2434256434
