Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
> - Updates, comment and variable cleanup
> - merge rest
> - merge
> - fixes and code comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1508:
> 1506: if (len > dst.remaining()) {
> 1507: throw new ShortBufferException("Output buffer too
> small, must" +
> 1508: "be at least " + (len - tagLenBytes) + " bytes
> long");
nit: add a space between "must" and "be".
Instead of '(len -tagLenBytes)' should just be 'len' as its initial value
already deducts tagLenBytes (see line 1495).
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
> - Updates, comment and variable cleanup
> - merge rest
> - merge
> - fixes and code comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1446:
> 1444: throw new AEADBadTagException("Input too short - need
> tag");
> 1445: }
> 1446:
Unrelated to your change, but I noticed that the if-block from line 1443-1445
should be moved up before the ArrayUtil.nullAndBoundsCheck(...), otherwise the
3rd argument may be negative and lead to a wrong exception being thrown.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Mon, 2 Aug 2021 19:31:54 GMT, Martin Balao wrote: > As described in JDK-8271566 [1], this patch proposal is intended to fix a > problem that arises when using DSA keys that have a 256-bits (or larger) G > parameter for signatures (either signing or verifying). There were some > incorrect assumptions and hard-coded length values in the code before. Please > note that, for example, the tuple (2048, 256) for DSA is valid according to > FIPS PUB 186-4. > > Beyond the specific issues in signatures, I decided to provide a broader > solution and enable key parameter retrieval for other key types (EC, DH) when > possible. This is, when the key is not sensitive. One thing that I should > note here is that token keys (those that have the CKA_TOKEN attribute equal > to 'true') are considered sensitive in this regard, at least by the NSS > Software Token implementation. I don't have access to other vendor > implementations but if there is any concern, we can adjust the constraint to > NSS-only. However, I'm not sure which use-case would require to get private > keys out of a real token, weakening its security. I'd be more conservative > here and not query the values if not sure that it will succeed. > > No regressions found in jdk/sun/security/pkcs11. A new test added: > LargerDSAKey. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 Yes, I see what you mean. Contrary to P11PrivateKey::getFormat and P11PrivateKey::getEncodedInternal where a 'null' returned value is documented in java.security.Key, we don't have that documentation for the other interfaces such as java.security.interfaces.DSAPrivateKey. That can lead to NPE if the client casts the P11Key to the interface, invokes the method and depends on a non-null value. I will give this some thought and try to come up with something, because the information is already there and (in reality) we need it internally only. It's clear that all these interfaces were not designed with unextractable P11 keys in mind, because it makes sense to me (conceptually) to have a private key from which we can retrieve some values (public, such as the parameters) and not other ones. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:57:11 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm >> - Updates, comment and variable cleanup >> - merge rest >> - merge >> - fixes and code comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 1120: > >> 1118: inOfs += r; >> 1119: inLen -= r; >> 1120: } > > Have you considered move the "if (inLen >= PARALLEL_LEN) block" into > EncryptOp.update() impl (just like the Encrypt.doFinal() impl) ? Even though > not all op.update() calls process large data, but it'd reduce code > duplication and ensures that all large data processed by EncryptOp.update() > calls would call the intrinsified method. There are cases where inLen is known to be smaller than PARALLEL_LEN and is a waste of a check, such as merging with the ibuffer to create one block. Also moving it into EncryptOp would always mean an additional check and maybe an unnecessary jump to another method. I did that for doFinal, because gctr/ghash.doFinal() needs to was no extra checks. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature
On Tue, 3 Aug 2021 21:05:24 GMT, Valerie Peng wrote:
>> As described in JDK-8271566 [1], this patch proposal is intended to fix a
>> problem that arises when using DSA keys that have a 256-bits (or larger) G
>> parameter for signatures (either signing or verifying). There were some
>> incorrect assumptions and hard-coded length values in the code before.
>> Please note that, for example, the tuple (2048, 256) for DSA is valid
>> according to FIPS PUB 186-4.
>>
>> Beyond the specific issues in signatures, I decided to provide a broader
>> solution and enable key parameter retrieval for other key types (EC, DH)
>> when possible. This is, when the key is not sensitive. One thing that I
>> should note here is that token keys (those that have the CKA_TOKEN attribute
>> equal to 'true') are considered sensitive in this regard, at least by the
>> NSS Software Token implementation. I don't have access to other vendor
>> implementations but if there is any concern, we can adjust the constraint to
>> NSS-only. However, I'm not sure which use-case would require to get private
>> keys out of a real token, weakening its security. I'd be more conservative
>> here and not query the values if not sure that it will succeed.
>>
>> No regressions found in jdk/sun/security/pkcs11. A new test added:
>> LargerDSAKey.
>>
>> --
>> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566
>
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java line
> 386:
>
>> 384: (attributes[2].getBoolean() == false)) {
>> 385: keySensitive = true;
>> 386: }
>
> nit: this block and line 377 can simply be:
> `boolean keySensitive = (attributes[0].getBoolean() ||
> attributes[1].getBoolean() || (!attributes[2].getBoolean()));`
Sure
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
> line 123:
>
>> 121:
>> 122: // signature length expected or 0 for unknown
>> 123: private int signatureLength;
>
> nit: use a shorter name, e.g. sigLen, so to fit in one line.
Ok
> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Signature.java
> line 817:
>
>> 815: }
>> 816:
>> 817: private byte[] asn1ToDSA(byte[] sig) throws SignatureException {
>
> Have you considered keeping this as a static method but add one more int
> argument, i.e. signature length? It seems that the method asn1ToECDSA() can
> be made static too.
Ok
> test/jdk/sun/security/pkcs11/Signature/LargeDSAKey.java line 52:
>
>> 50: "Known text known text known text";
>> 51:
>> 52: public void main(Provider p) throws Exception {
>
> nit: add @Override
Sure
-
PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
> - Updates, comment and variable cleanup
> - merge rest
> - merge
> - fixes and code comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1608:
> 1606: int bufRemainder = bLen - len;
> 1607: if (bufRemainder >= blockSize) {
> 1608: resultLen = op.update(buffer, len, bufRemainder,
> out, outOfs);
nit: exceeds 80 chars
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1721:
> 1719:
> 1720: if (inLen >= PARALLEL_LEN) {
> 1721: len = implGCMCrypt(in, inOfs, inLen, out, outOfs, out,
> outOfs, gctr, ghash);
nit: exceeds 80 chars
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 20:01:00 GMT, Valerie Peng wrote:
>> Smita Kamath has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>> - Updates, comment and variable cleanup
>> - merge rest
>> - merge
>> - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 1721:
>
>> 1719:
>> 1720: if (inLen >= PARALLEL_LEN) {
>> 1721: len = implGCMCrypt(in, inOfs, inLen, out, outOfs, out,
>> outOfs, gctr, ghash);
>
> nit: exceeds 80 chars
The editor keeps reverting these for some reason.. it's very annoying
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with five additional > commits since the last revision: > > - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm > - Updates, comment and variable cleanup > - merge rest > - merge > - fixes and code comments src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1120: > 1118: inOfs += r; > 1119: inLen -= r; > 1120: } Have you considered move the "if (inLen >= PARALLEL_LEN) block" into EncryptOp.update() impl (just like the Encrypt.doFinal() impl) ? Even though not all op.update() calls process large data, but it'd reduce code duplication and ensures that all large data processed by EncryptOp.update() calls would call the intrinsified method. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 18:40:44 GMT, Valerie Peng wrote:
>> Smita Kamath has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>> - Updates, comment and variable cleanup
>> - merge rest
>> - merge
>> - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 1473:
>
>> 1471: if (mismatch != 0) {
>> 1472: // Clear output data
>> 1473: Arrays.fill(out, outOfs, outOfs + len, (byte) 0);
>
> Update the javadoc of this method (line 1428-1429) with this change, i.e.
> clears output data if tag verification fails. Same goes for line 1482-1483.
ok
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:44:05 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm >> - Updates, comment and variable cleanup >> - merge rest >> - merge >> - fixes and code comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 87: > >> 85: private static final int MAX_BUF_SIZE = Integer.MAX_VALUE; >> 86: // data size when buffer is divided up to aid in intrinsics >> 87: private static final int TRIGGERLEN = 65536; // 64k > > With this interleaved impl, is this TRIGGERLEN still needed? The > implGCMCrypt(byte[] in, int inOfs, int inLen, > byte[] ct, int ctOfs, byte[] out, int outOfs, GCTR gctr, GHASH ghash) > method is intrinsified, would there be a difference in increasing the number > of gctr/ghash calls inside an already intrinsified method? Yes, they are two different intrinsics. The new implGCMCrypt intrinsic is supported by newer processors so there is no guarantee that implGCMCrypt will run the intrinsic. - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:32:25 GMT, Valerie Peng wrote: >> Smita Kamath has updated the pull request incrementally with five additional >> commits since the last revision: >> >> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm >> - Updates, comment and variable cleanup >> - merge rest >> - merge >> - fixes and code comments > > src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java > line 1648: > >> 1646: >> 1647: ghash.doFinal(in, inOfs, inLen); >> 1648: return len + gctr.doFinal(in, inOfs, inLen, out, outOfs); > > Why not just keep "return len + op.doFinal(in, inOfs, inLen, out, outOfs);"? It probably got changed because I was cleaning up unused 'op' elsewhere - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with five additional > commits since the last revision: > > - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm > - Updates, comment and variable cleanup > - merge rest > - merge > - fixes and code comments src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 87: > 85: private static final int MAX_BUF_SIZE = Integer.MAX_VALUE; > 86: // data size when buffer is divided up to aid in intrinsics > 87: private static final int TRIGGERLEN = 65536; // 64k With this interleaved impl, is this TRIGGERLEN still needed? The implGCMCrypt(byte[] in, int inOfs, int inLen, byte[] ct, int ctOfs, byte[] out, int outOfs, GCTR gctr, GHASH ghash) method is intrinsified, would there be a difference in increasing the number of gctr/ghash calls inside an already intrinsified method? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote: >> I would like to submit AES-GCM optimization for x86_64 architectures >> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES >> and GHASH operations. >> Performance gain of ~1.5x - 2x for message sizes 8k and above. > > Smita Kamath has updated the pull request incrementally with five additional > commits since the last revision: > > - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm > - Updates, comment and variable cleanup > - merge rest > - merge > - fixes and code comments src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 1648: > 1646: > 1647: ghash.doFinal(in, inOfs, inLen); > 1648: return len + gctr.doFinal(in, inOfs, inLen, out, outOfs); Why not just keep "return len + op.doFinal(in, inOfs, inLen, out, outOfs);"? - PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Fri, 6 Aug 2021 19:16:39 GMT, Valerie Peng wrote:
>> Smita Kamath has updated the pull request incrementally with five additional
>> commits since the last revision:
>>
>> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
>> - Updates, comment and variable cleanup
>> - merge rest
>> - merge
>> - fixes and code comments
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java
> line 1779:
>
>> 1777: int len = 0;
>> 1778: if (inLen >= PARALLEL_LEN) {
>> 1779: implGCMCrypt(in, inOfs, inLen, in, inOfs, out, outOfs,
>> gctr,
>
> Should save the return value into 'len'? For consistency sake, choose between
> GaloisCounterMode.implGCMCrypt(...) and implGCMCrypt and not both?
I do not understand this comment
-
PR: https://git.openjdk.java.net/jdk/pull/4019
RFR: 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup
I'd like to propose a fix for JDK-8270137 [1]. This bug is triggered when using a previously stored referral ticket (in the Referrals Cache) at the moment of following a S4U2Proxy cross-realm referral. The mistakenly-used referral ticket matched the client and service names but it was obtained as a result of a non-S4U2Proxy request. In fact, it was the middle service that got it while trying to determine the backend service realm in a previous S4U2Proxy communication. The mistakenly-used referral ticket was not bind to the impersonated user (in other words, it was not obtained attaching the user's TGS as part of a S4U2Proxy request) and, thus, must not be used. Even when one possible approach to fix this issue could be to be more selective at the moment of getting referral tickets from the Cache (that is: do not get anything from the Cache if it's for a S4U2Proxy request), I decided to go one step further and enhance the Referrals Cache. With this enhancement, we add more information to the stored referral tickets such as a footprint of the TGS (in the case of S4U2Proxy requests) or the user principal (in the case of S4U2Self requests). We now allow to store S4U2Proxy and S4U2Self referrals tickets but those will be re-used only if there is a perfect match of the TGS or user principal. As an example, if a middle service tries to replicate the exact S4U2Self communication for exactly the same user, cached referral tickets should be okay. With this enhancement, we increase the use of the Cache and the performance (time, network resources, etc.). The ReferralsTest is enhanced to reflect these new scenarios and now uses cached S4U2Proxy/S4U2Self referral tickets. No regressions observed in jdk/sun/security/krb5. -- [1] - https://bugs.openjdk.java.net/browse/JDK-8270137 - Commit messages: - 8270137: Kerberos Credential Retrieval from Cache not Working in Cross-Realm Setup Changes: https://git.openjdk.java.net/jdk/pull/5036/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5036&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270137 Stats: 89 lines in 3 files changed: 47 ins; 9 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/5036.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5036/head:pull/5036 PR: https://git.openjdk.java.net/jdk/pull/5036
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
> - Updates, comment and variable cleanup
> - merge rest
> - merge
> - fixes and code comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1779:
> 1777: int len = 0;
> 1778: if (inLen >= PARALLEL_LEN) {
> 1779: implGCMCrypt(in, inOfs, inLen, in, inOfs, out, outOfs,
> gctr,
Should save the return value into 'len'?
-
PR: https://git.openjdk.java.net/jdk/pull/4019
Re: RFR: 8267125: AES Galois CounterMode (GCM) interleaved implementation using AVX512 + VAES instructions [v7]
On Wed, 4 Aug 2021 02:24:05 GMT, Smita Kamath wrote:
>> I would like to submit AES-GCM optimization for x86_64 architectures
>> supporting AVX3+VAES (Evex encoded AES). This optimization interleaves AES
>> and GHASH operations.
>> Performance gain of ~1.5x - 2x for message sizes 8k and above.
>
> Smita Kamath has updated the pull request incrementally with five additional
> commits since the last revision:
>
> - Merge branch 'aes-gcm' of github.com:smita-kamath/jdk into aes-gcm
> - Updates, comment and variable cleanup
> - merge rest
> - merge
> - fixes and code comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1473:
> 1471: if (mismatch != 0) {
> 1472: // Clear output data
> 1473: Arrays.fill(out, outOfs, outOfs + len, (byte) 0);
Update the javadoc of this method (line 1428-1429) with this change, i.e.
clears output data if tag verification fails. Same goes for line 1482-1483.
-
PR: https://git.openjdk.java.net/jdk/pull/4019
