On Fri, 17 Oct 2025 05:41:25 GMT, Valerie Peng <[email protected]> wrote:
>> 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 911:
>
>> 909: }
>> 910: sessionK[0] = genRoundKeys(key, rounds);
>> 911: sessionK[1] = invGenRoundKeys();
>
> Given the decryption round keys are somewhat based on the encryption round
> keys, we could combine these two methods into one, e.g.
>
> private static int[][] genRoundKeys(byte[] key, int rounds) {
> int[][] ks = new int[2][]; // key schedule
>
> int wLen = (rounds + 1) * WB;
> int nk = key.length / WB;
>
> // generate the round keys for encryption
> int[] w = new int[wLen];
> for (int i = 0, j = 0; i < nk; i++, j+=4) {
> w[i] = ((key[j] & 0xFF) << 24)
> | ((key[j + 1] & 0xFF) << 16)
> | ((key[j + 2] & 0xFF) << 8)
> | (key[j + 3] & 0xFF);
> }
> for (int i = nk; i < wLen; i++) {
> int tmp = w[i - 1];
> if (i % nk == 0) {
> int rW = (tmp << 8) & 0xFFFFFF00 | (tmp >>> 24);
> tmp = subWord(rW) ^ RCON[(i / nk) - 1];
> } else if ((nk > 6) && ((i % nk) == WB)) {
> tmp = subWord(tmp);
> }
> w[i] = w[i - nk] ^ tmp;
> }
> ks[0] = w;
>
> // generate the decryption round keys based on encryption ones
> int[] dw = new int[wLen];
> int[] temp = new int[WB];
>
> // Intrinsics requires the inverse key expansion to be reverse order
> // except for the first and last round key as the first two round keys
> // are without a mix column transform.
> for (int i = 1; i < rounds; i++) {
> System.arraycopy(w, i * WB, temp, 0, WB);
> invMixRKey(temp);
> System.arraycopy(temp, 0, dw, wLen - (i * WB), WB);
> }
> // dw[0...3] <- w[0...3] AND dw[4...7] <- w[(wLen - 4)...(wLen -1)]
> System.arraycopy(w, 0, dw, 0, WB);
> System.arraycopy(w, wLen - WB, dw, WB, WB);
> ks[1] = dw;
> Arrays.fill(temp, 0);
>
> return ks;
> }
These two methods were only the few that I was able to make that were compact
and singular in purpose (gen round key, gen inverse round key) code as the
coding style guidelines espouse. The rest of the methods' construction were
dictated by performance improvements, where compactness came at the cost of
interpreter speed.
> 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]);
> }
I've removed this method and inlined this logic in the invGenRoundKeys method.
> src/java.base/share/classes/com/sun/crypto/provider/AES_Crypt.java line 976:
>
>> 974: * @param state [in, out] the round key for inverse mix column
>> processing.
>> 975: */
>> 976: private static void invMixRKey(int[] state) {
>
> nit: name the method "invMixColumns(int[])". This name matches the spec
> psuedo code and goes better with the "state" argument name. Or use
> "invMixRoundKey(int[] roundKey)"?
I've removed this method and inlined this logic in the invGenRoundKeys method.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2438587221
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2438587085
PR Review Comment: https://git.openjdk.org/jdk/pull/27807#discussion_r2438586207
