Bug 1663661 - Guard against NULL token in nssSlot_IsTokenPresent. r=jcj
authorKevin Jacobs <kjacobs@mozilla.com>
Tue, 03 Nov 2020 19:02:15 +0000
changeset 15788 0ed11a5835ac1556ff978362cd61069d48f4c5db
parent 15787 424974716ef0af19fc1c1c865f4415e64d55dd67
child 15789 3ce42ead87f9b629de37d91068f65e7006538ebc
push id3867
push userkjacobs@mozilla.com
push dateTue, 03 Nov 2020 19:07:41 +0000
reviewersjcj
bugs1663661
Bug 1663661 - Guard against NULL token in nssSlot_IsTokenPresent. r=jcj This patch addresses locking inconsistency in `nssSlot_IsTokenPresent` by retaining the slot lock for the duration of accesses to `slot->token`. This is already done correctly elsewhere. As a side effect, this introduces an ordering requirement: we take `slot->lock` followed by `session->lock`. Differential Revision: https://phabricator.services.mozilla.com/D95636
lib/dev/devslot.c
lib/dev/devt.h
--- a/lib/dev/devslot.c
+++ b/lib/dev/devslot.c
@@ -166,29 +166,30 @@ nssSlot_IsTokenPresent(
      * if the token is present and that it needs initialization. */
     slot->lastTokenPingState = nssSlotLastPingState_Update;
     slot->isPresentThread = PR_GetCurrentThread();
 
     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 */
+        if (slot->token) {
+            slot->token->base.name[0] = 0; /* XXX */
+        }
         isPresent = PR_FALSE;
-        goto done;
+        goto done; /* slot lock held */
     }
     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 */
             isPresent = PR_FALSE;
-            goto done;
+            goto done; /* slot lock held */
         }
         session = nssToken_GetDefaultSession(slot->token);
         if (session) {
             nssSession_EnterMonitor(session);
             /* token is not present */
             if (session->handle != CK_INVALID_HANDLE) {
                 /* session is valid, close and invalidate it */
                 CKAPI(epv)
@@ -201,18 +202,26 @@ nssSlot_IsTokenPresent(
             /* 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);
         isPresent = PR_FALSE;
-        goto done;
+        goto done; /* slot lock held */
     }
+    if (!slot->token) {
+        /* This should not occur, based on the fact that the
+         * below calls will dereference NULL otherwise. */
+        PORT_Assert(0);
+        isPresent = PR_FALSE;
+        goto done; /* slot lock held */
+    }
+
     /* 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 tokenRemoved;
         nssSession_EnterMonitor(session);
         if (session->handle != CK_INVALID_HANDLE) {
@@ -225,17 +234,17 @@ nssSlot_IsTokenPresent(
                 session->handle = CK_INVALID_HANDLE;
             }
         }
         tokenRemoved = (session->handle == CK_INVALID_HANDLE);
         nssSession_ExitMonitor(session);
         /* token not removed, finished */
         if (!tokenRemoved) {
             isPresent = PR_TRUE;
-            goto done;
+            goto done; /* slot lock held */
         }
     }
     /* 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);
@@ -243,16 +252,17 @@ nssSlot_IsTokenPresent(
     nssrv = nssSlot_Refresh(slot);
     isPresent = PR_TRUE;
     if (nssrv != PR_SUCCESS) {
         slot->token->base.name[0] = 0; /* XXX */
         slot->ckFlags &= ~CKF_TOKEN_PRESENT;
         isPresent = PR_FALSE;
     }
 done:
+    nssSlot_ExitMonitor(slot);
     /* Once we've set up the condition variable,
      * Before returning, it's necessary to:
      *  1) Set the lastTokenPingTime 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.
      */
--- a/lib/dev/devt.h
+++ b/lib/dev/devt.h
@@ -91,16 +91,19 @@ struct NSSSlotStr {
     void *epv;
     PK11SlotInfo *pk11slot;
     PZLock *isPresentLock;
     PRCondVar *isPresentCondition;
     PRThread *isPresentThread;
 };
 
 struct nssSessionStr {
+    /* Must not hold slot->lock when taking lock.
+     * See ordering in nssSlot_IsTokenPresent.
+     */
     PZLock *lock;
     CK_SESSION_HANDLE handle;
     NSSSlot *slot;
     PRBool isRW;
     PRBool ownLock;
 };
 
 typedef enum {