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 954: > 952: } > 953: w[i] = w[i - nk] ^ tmp; > 954: } Looks like most of these local variables can be removed? Since you are not changing the value of `len`, you can just use `WB`. `rW` is only used inside the if-block from line 944-948, so it can be declared on line 945. Line 946-948 can be merged on one line, e.g. `tmp = subByte(rW, SBOX) ^ RCON[(i / nk) - 1];` and no need for `subWord` and `g`. Same goes for line 950 and 951. Also, the value of `WB * (rounds + 1)` is used twice, this can be stored in a local variable say `wLen`, so it's only calculated once. Same goes for the `i * WB` value from line 937-940 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2434276514
