On Thu, 16 Oct 2025 20:22:20 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 958:
> 956: * @return the processed round key row.
> 957: */
> 958: private static int invMix(int[] state, int idx) {
It seems that we can just use an `int` argument and make the callers do the
array dereferencing. This way we can get rid of the temporary buffer inside
`invMixRKey(int[])` as passing an integer to `invMix(int)` method will not
affect the array, e.g.
private static void invMixRKey(int[] state) {
state[0] = invMix(state[0]);
state[1] = invMix(state[1]);
state[2] = invMix(state[2]);
state[3] = invMix(state[3]);
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2438501716
