<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Hi Martin,</p>
<p>For the KeyStore case, they are mostly token objects which the
extract key info approach does not apply?</p>
<p>For your changes in p11_keymgmt.c, I ran into compiler error and
SIGBUS errors on two OS (mac and solaris sparc), I ended up
changing variable initializations as well as memset(...). With the
updated native changes, I adapted and re-tested my prototype
changes. For reference, you can find the updated prototype changes
at: <a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.01/">http://cr.openjdk.java.net/~valeriep/6913047Exp/webrev.01/</a></p>
<p>Besides making changes in the keymgmet.c for getting rid of
platform-specific compilation error and SIGBUS error, I noticed
that you hardcoded the key wrapping mechanism in native code for
both getNativeKeyInfo(...)/createNativeKey() methods, it seems
better to storing the mechanism object at java side, i.e. P11Key
and its associated classes, and then pass the object to JNI code
(please also see my webrev.01)<br>
</p>
<p>In addition, I switched the reference counting to your model,
i.e. increase in init() and decrease in reset(), instead of the
try-n-finally model in prototype webrev.00. My earlier comment on
P11Cipher class which you should not replace the initialize() call
with ensureInitialized() call applies to all other PKCS11 classes
as well.</p>
<p>With this approach, the KeyID field of P11Key should not be
freely accessible and directly referenced outside of P11Key class.
Also, the increase and decrease of reference counting must be
paired up. Supposedly, the reference count should not go negative,
right? If the reference counting isn't correct, the key may be
freed pre-maturely? Lastly, the reference counting is an
implementation detail and I think it's better to keep it inside
the P11Key class/file and not exposing it, i.e. through method
names.</p>
<p>I have spent time verifying my updated prototype changes and
trace the reference counting. All look fine, except there is one
regression test failure (sun/security/tools/keytool/NssTest.java)
on linux-x64 which I am still troubleshooting. However, I will be
on vacation from 10/4 to 10/21, so I want to update you on what I
have so you can continue during my vacation.</p>
<p>Thanks,<br>
</p>
<p>Valerie<br>
</p>
<p> <br>
</p>
<br>
<div class="moz-cite-prefix">On 9/18/2018 4:48 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:540e790d-1edf-732e-bb15-85537a6aa9c8@oracle.com">
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<p>Hi Martin,</p>
<p>I am ok with your conservation choice of only applying this
when using NSS. If we are only applying this for NSS, we should
really refactor the code to minimize the impact on callers and
P11Key class. My prototype code may be on the extreme end of
minimizing changes. But the current webrev can use some
refactoring also. With your explanation, I now understand your
model better. How about the refactoring in P11Key class? Is
there a reason for not doing this? I did test my prototype code
against existing regression tests (except the KeyStore ones as
more API changes are needed for persistent keys which I have not
covered in prototype) but I ran into some strange errors in some
native p11 calls <span class="new">which I did not touch so I
commented them out and just checked the part of reference
count, etc.<br>
</span></p>
I will take a closer look at the KeyStore case and let you know.<br>
Thanks,<br>
Valerie<br>
<br>
<div class="moz-cite-prefix">On 9/18/2018 7:29 AM, Martin Balao
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAKZz+gd8ARwjKd0Pu6tpXXCNStuTY8nc5ObmXEYKqawcezkwDQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=utf-8">
<div dir="ltr">
<div dir="ltr">
<div>Hi Valerie,</div>
<div><br>
</div>
<div>Thanks for your comments.</div>
<div><br>
</div>
<div>Here it is Webrev.11:</div>
<div><br>
</div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.11/"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11/</a></div>
<div> * <a
href="http://cr.openjdk.java.net/%7Embalao/webrevs/6913047/6913047.webrev.11.zip"
moz-do-not-send="true">http://cr.openjdk.java.net/~mbalao/webrevs/6913047/6913047.webrev.11.zip</a></div>
<div><br>
</div>
<div><src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java></div>
<div>L397: That's right. I was trying to simplify the code
but missed this. Thanks.</div>
<div>L471: The key reference counter has to be decremented
under any exception (P11Key.decNativeKeyRef method call).
But, yes, no exception different than PKCS11Exception
should be thrown. Reverted this change.</div>
<div><br>
</div>
<div><src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Key.java></div>
<div>L99: Comment changed. It should be better now.</div>
<div>L148-L149: In fact, I'd enforce this and disable the
feature for all token keys. Token keys are permanent and
extracting them is risky. This criteria was already
applied when dealing with key stores (P11Keystore class).</div>
<div><br>
</div>
<div>Yes, this feature is enabled for NSS only because it's
the only backend we currently know that is affected by
this memory "leak" issue. If there were any other
software-token backend affected, we can try this feature
there too. HSMs shouldn't have any problem. I prefer to
take a more conservative approach and enable the feature
only in those cases in which it's really necessary. All
other cases, default to the previous mechanism for freeing
memory.</div>
<div><br>
</div>
<div>This does not replace the PhantomReference approach;
both work together and are complementary. In cases where
temporary keys feature is disabled or when a temporary key
client is not behaving correctly (i.e.: leaking stateful
operations like "cipher" or "signature" in an intermediate
state with the native key initialized), PhantomReference
approach will be the last chance to free memory. The
native key object can be destroyed (C_DestroyObject call)
either from the PhantomReference mechanism or from the
temporary keys mechanism. There shouldn't be any conflict
between them. If it's destroyed through temporary keys
mechanism, then we know that the P11Key object is alive
(refereced) and thus PhantomReference destruction won't be
taking place at the same time. Once the key is deleted,
keyID is set to 0 and session to null. Thus,
PhantomReference destruction won't have any effect when
executed later. If we think of the other case (when the
key is freed by PhantomReference), we have a P11Key object
with a native key initialized but with no references to
it. Thus, destroyNativeKey method won't be called and
SessionKeyRef.disposeNative is the only method that will
delete the key.</div>
<div><br>
</div>
<div>L157: that's right, synchronization has to be at class
level. Fixed.</div>
<div><br>
</div>
<div>L1343: It's not the same session: this.session was
assigned a new value (this.session = session;) before
calling addObject.</div>
<div><br>
</div>
<div>L1363: removeObject is called for the session, inside
setKeyIDAndSession: "this.session.removeObject();". Null
is set to this.session instance variable after this call.</div>
<div><br>
</div>
<div>In regards to the refactorings you proposed, the
problem I see with moving key reference
incrementing/decrementing to PKCS11.java is that some
operations are stateful. I.e.: encryption. When we
initialize the operation with C_EncryptInit, the key id is
the 3rd parameter. Destroying the key id and then doing
C_EncryptUpdate sounds incorrect to me. Have you tried the
regression testing suite after this refactoring? (I see
some parts commented). In regards to removing the
tmpNativeKey parameter (used to explicitly disable the
feature for new P11Key objects), how do you handle the
P11KeyStore case? We don't want temporary keys there.</div>
<div><br>
</div>
<div>Kind regards,</div>
<div>Martin.-</div>
<div><br>
</div>
</div>
</div>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>