Crash in PK11_DoesMechanism due to race condition
☠☠ backed out by 7a3ff26a6b12 ☠ ☠
authorRobert Relyea <rrelyea@redhat.com>
Wed, 03 Jan 2018 16:28:05 -0800
changeset 14200 919051b7381e1e7e36c92ed0bbbbbbd7bd71a588
parent 14199 82aff3e2439e4396e5f25c88900520bdb8b5128f
child 14201 7a3ff26a6b12e3ca7cf60f89f6d037b514458e85
push id2945
push userrrelyea@redhat.com
push dateThu, 04 Jan 2018 00:28:25 +0000
bugs1054373
Crash in PK11_DoesMechanism due to race condition bug 1054373 r=rsleevi
lib/dev/devslot.c
lib/dev/devt.h
lib/pk11wrap/dev3hack.c
--- a/lib/dev/devslot.c
+++ b/lib/dev/devslot.c
@@ -28,16 +28,18 @@ static PRIntervalTime s_token_delay_time
 NSS_IMPLEMENT PRStatus
 nssSlot_Destroy(
     NSSSlot *slot)
 {
     if (slot) {
         if (PR_ATOMIC_DECREMENT(&slot->base.refCount) == 0) {
             PK11_FreeSlot(slot->pk11slot);
             PZ_DestroyLock(slot->base.lock);
+            PZ_DestroyCondVar(slot->isPresentCondition);
+            PZ_DestroyLock(slot->isPresentLock);
             return nssArena_Destroy(slot->base.arena);
         }
     }
     return PR_SUCCESS;
 }
 
 void
 nssSlot_EnterMonitor(NSSSlot *slot)
@@ -102,55 +104,78 @@ within_token_delay_period(const NSSSlot 
     time = PR_IntervalNow();
     lastTime = slot->lastTokenPing;
     if ((lastTime) && ((time - lastTime) < s_token_delay_time)) {
         return PR_TRUE;
     }
     return PR_FALSE;
 }
 
+
 NSS_IMPLEMENT PRBool
 nssSlot_IsTokenPresent(
     NSSSlot *slot)
 {
     CK_RV ckrv;
     PRStatus nssrv;
     /* XXX */
     nssSession *session;
     CK_SLOT_INFO slotInfo;
     void *epv;
+    PRBool isPresent = PR_FALSE;
+
     /* permanent slots are always present unless they're disabled */
     if (nssSlot_IsPermanent(slot)) {
         return !PK11_IsDisabled(slot->pk11slot);
     }
+
     /* avoid repeated calls to check token status within set interval */
     if (within_token_delay_period(slot)) {
         return ((slot->ckFlags & CKF_TOKEN_PRESENT) != 0);
     }
 
-    /* First obtain the slot info */
+    /* First obtain the slot epv before we set up the condition
+     * variable, so we can just return if we couldn't get it. */
     epv = slot->epv;
     if (!epv) {
         return PR_FALSE;
     }
+
+    /* set up condition so only one thread is active in this part of the code at a time */
+    PZ_Lock(slot->isPresentLock);
+    while (slot->inIsPresent) {
+        PR_WaitCondVar(slot->isPresentCondition, 0);
+    }
+    /* if we were one of multiple threads here, the first thread will have
+     * given us the answer, no need to make more queries of the token. */
+    if (within_token_delay_period(slot)) {
+	CK_FLAGS ckFlags = slot->ckFlags;
+        PZ_Unlock(slot->isPresentLock);
+        return ((ckFlags & CKF_TOKEN_PRESENT) != 0);
+    }
+    /* this is the winning thread, block all others until we've determined
+     * if the token is present and that it needs initialization. */
+    slot->inIsPresent = PR_TRUE;
+    PZ_Unlock(slot->isPresentLock);
+
     nssSlot_EnterMonitor(slot);
     ckrv = CKAPI(epv)->C_GetSlotInfo(slot->slotID, &slotInfo);
     nssSlot_ExitMonitor(slot);
     if (ckrv != CKR_OK) {
         slot->token->base.name[0] = 0; /* XXX */
-        slot->lastTokenPing = PR_IntervalNow();
-        return PR_FALSE;
+	isPresent = PR_FALSE;
+	goto done;
     }
     slot->ckFlags = slotInfo.flags;
     /* check for the presence of the token */
     if ((slot->ckFlags & CKF_TOKEN_PRESENT) == 0) {
         if (!slot->token) {
             /* token was never present */
-            slot->lastTokenPing = PR_IntervalNow();
-            return PR_FALSE;
+	    isPresent = PR_FALSE;
+	    goto done;
         }
         session = nssToken_GetDefaultSession(slot->token);
         if (session) {
             nssSession_EnterMonitor(session);
             /* token is not present */
             if (session->handle != CK_INVALID_SESSION) {
                 /* session is valid, close and invalidate it */
                 CKAPI(epv)
@@ -162,18 +187,18 @@ nssSlot_IsTokenPresent(
         if (slot->token->base.name[0] != 0) {
             /* notify the high-level cache that the token is removed */
             slot->token->base.name[0] = 0; /* XXX */
             nssToken_NotifyCertsNotVisible(slot->token);
         }
         slot->token->base.name[0] = 0; /* XXX */
         /* clear the token cache */
         nssToken_Remove(slot->token);
-        slot->lastTokenPing = PR_IntervalNow();
-        return PR_FALSE;
+	isPresent = PR_FALSE;
+	goto done;
     }
     /* token is present, use the session info to determine if the card
      * has been removed and reinserted.
      */
     session = nssToken_GetDefaultSession(slot->token);
     if (session) {
         PRBool isPresent = PR_FALSE;
         nssSession_EnterMonitor(session);
@@ -186,37 +211,49 @@ nssSlot_IsTokenPresent(
                     ->C_CloseSession(session->handle);
                 session->handle = CK_INVALID_SESSION;
             }
         }
         isPresent = session->handle != CK_INVALID_SESSION;
         nssSession_ExitMonitor(session);
         /* token not removed, finished */
         if (isPresent) {
-            slot->lastTokenPing = PR_IntervalNow();
-            return PR_TRUE;
+	    isPresent = PR_TRUE;
+	    goto done;
         }
     }
     /* the token has been removed, and reinserted, or the slot contains
      * a token it doesn't recognize. invalidate all the old
      * information we had on this token, if we can't refresh, clear
      * the present flag */
     nssToken_NotifyCertsNotVisible(slot->token);
     nssToken_Remove(slot->token);
     /* token has been removed, need to refresh with new session */
     nssrv = nssSlot_Refresh(slot);
+    isPresent = PR_TRUE;
     if (nssrv != PR_SUCCESS) {
         slot->token->base.name[0] = 0; /* XXX */
         slot->ckFlags &= ~CKF_TOKEN_PRESENT;
-        /* TODO: insert a barrier here to avoid reordering of the assingments */
-        slot->lastTokenPing = PR_IntervalNow();
-        return PR_FALSE;
+	isPresent = PR_FALSE;
     }
-    slot->lastTokenPing = PR_IntervalNow();
-    return PR_TRUE;
+done:
+    /* Once we've set up the condition variable,
+     * Before returning, it's necessary to:
+     *  1) Set the lastTokenPing time so that any other threads waiting on this
+     *     initialization and any future calls within the initialization window
+     *     return the just-computed status.
+     *  2) Indicate we're complete, waking up all other threads that may still
+     *     be waiting on initialization can progress.
+     */
+     slot->lastTokenPing = PR_IntervalNow();
+     PZ_Lock(slot->isPresentLock);
+     slot->inIsPresent = PR_FALSE;
+     PR_NotifyAllCondVar(slot->isPresentCondition);
+     PZ_Unlock(slot->isPresentLock);
+     return isPresent;
 }
 
 NSS_IMPLEMENT void *
 nssSlot_GetCryptokiEPV(
     NSSSlot *slot)
 {
     return slot->epv;
 }
@@ -224,17 +261,17 @@ nssSlot_GetCryptokiEPV(
 NSS_IMPLEMENT NSSToken *
 nssSlot_GetToken(
     NSSSlot *slot)
 {
     NSSToken *rvToken = NULL;
 
     if (nssSlot_IsTokenPresent(slot)) {
         /* Even if a token should be present, check `slot->token` too as it
-	 * might be gone already. This would happen mostly on shutdown. */
+         * might be gone already. This would happen mostly on shutdown. */
         nssSlot_EnterMonitor(slot);
         if (slot->token)
             rvToken = nssToken_AddRef(slot->token);
         nssSlot_ExitMonitor(slot);
     }
 
     return rvToken;
 }
--- a/lib/dev/devt.h
+++ b/lib/dev/devt.h
@@ -76,16 +76,19 @@ struct NSSSlotStr {
     NSSToken *token;   /* Peer */
     CK_SLOT_ID slotID;
     CK_FLAGS ckFlags; /* from CK_SLOT_INFO.flags */
     struct nssSlotAuthInfoStr authInfo;
     PRIntervalTime lastTokenPing;
     PZLock *lock;
     void *epv;
     PK11SlotInfo *pk11slot;
+    PZLock *isPresentLock;
+    PRCondVar *isPresentCondition;
+    PRBool inIsPresent;
 };
 
 struct nssSessionStr {
     PZLock *lock;
     CK_SESSION_HANDLE handle;
     NSSSlot *slot;
     PRBool isRW;
     PRBool ownLock;
--- a/lib/pk11wrap/dev3hack.c
+++ b/lib/pk11wrap/dev3hack.c
@@ -115,16 +115,19 @@ nssSlot_CreateFromPK11SlotInfo(NSSTrustD
     rvSlot->base.lock = PZ_NewLock(nssILockOther);
     rvSlot->base.arena = arena;
     rvSlot->pk11slot = PK11_ReferenceSlot(nss3slot);
     rvSlot->epv = nss3slot->functionList;
     rvSlot->slotID = nss3slot->slotID;
     /* Grab the slot name from the PKCS#11 fixed-length buffer */
     rvSlot->base.name = nssUTF8_Duplicate(nss3slot->slot_name, td->arena);
     rvSlot->lock = (nss3slot->isThreadSafe) ? NULL : nss3slot->sessionLock;
+    rvSlot->isPresentLock = PZ_NewLock(nssiLockOther);
+    rvSlot->isPresentCondition = PR_NewCondVar(rvSlot->isPresentLock);
+    rvSlot->inIsPresent = PR_FALSE;
     return rvSlot;
 }
 
 NSSToken *
 nssToken_CreateFromPK11SlotInfo(NSSTrustDomain *td, PK11SlotInfo *nss3slot)
 {
     NSSToken *rvToken;
     NSSArena *arena;