libafs: Avoid using changing unixuser ticket data

PSetTokens was afs_osi_Alloc'ing after afs_osi_Free'ing the previous
token data. This can sleep, causing tu->stp to be pointing to garbage
while we wait to alloc. Additionally, rxkad_NewClientSecurityObject
can sleep while waiting to alloc memory, and so the given tu->stp
pointer given to it by afs_ConnBySA may be invalid by the time it
actually uses the data.

To fix this, we could implement unixuser locking to ensure mutual
exclusion of these events. However, this implements a more
conservative change for the 1.4 and 1.6 branches. In PSetTokens we
alloc the new memory before we change anything, and in afs_ConnBySA we
make copies of the ticket data before giving it to rxkad. With these
changes, the glock gives us enough serialization to avoid issues with
tu->stp changing underneath us.

This change is specific to 1.4 and 1.6. On the master branch, this
issue is fixed by implementing unixuser locks in change
Idd66d72f716b7e7dc08faa31ae43e9a23639bae3.

Reviewed-on: http://gerrit.openafs.org/4649
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Derrick Brashear <shadow@dementia.org>
(cherry picked from commit 1465946bb6863430bf0efebd024d394549a8775f)

Change-Id: Icab5176bf685c408447f0f32ad65c5b003299d3d
Reviewed-on: http://gerrit.openafs.org/6345
Reviewed-by: Derrick Brashear <shadow@dementix.org>
Tested-by: BuildBot <buildbot@rampaginggeek.com>

diff --git a/src/afs/afs_conn.c b/src/afs/afs_conn.c
index ffcd132da0c4f8e0fa1c47e5601244e4f398b882..3de6c18dd1087f59c95ab0e4dd0894f6858fbd03 100644 (file)
--- a/src/afs/afs_conn.c
+++ b/src/afs/afs_conn.c
@@ -72,13 +72,27 @@ afs_pickSecurityObject(struct afs_conn *conn, int *secLevel)
 
     /* Do we have tokens ? */
     if (conn->user->vid != UNDEFVID) {
+       char *ticket;
+       struct ClearToken ct;
+
        *secLevel = 2;
+
+       /* Make a copy of the ticket data to give to rxkad, because the
+        * the ticket data could change while rxkad is sleeping for memory
+        * allocation. We should implement locking on unixuser
+        * structures to fix this properly, but for now, this is easier. */
+       ticket = afs_osi_Alloc(MAXKTCTICKETLEN);
+       memcpy(ticket, conn->user->stp, conn->user->stLen);
+       memcpy(&ct, &conn->user->ct, sizeof(ct));
+
        /* kerberos tickets on channel 2 */
        secObj = rxkad_NewClientSecurityObject(
                    cryptall ? rxkad_crypt : rxkad_clear,
-                    (struct ktc_encryptionKey *)conn->user->ct.HandShakeKey,
-                   conn->user->ct.AuthHandle,
-                   conn->user->stLen, conn->user->stp);
+                    (struct ktc_encryptionKey *)ct.HandShakeKey,
+                   ct.AuthHandle,
+                   conn->user->stLen, ticket);
+
+       afs_osi_Free(ticket, MAXKTCTICKETLEN);
      }
      if (secObj == NULL) {
        *secLevel = 0;

diff --git a/src/afs/afs_pioctl.c b/src/afs/afs_pioctl.c
index bc6f5e24f9422eec17fc05a3e63270aeec5a5d97..c3ff965d439b7125902b5da3bda64f3618610366 100644 (file)
--- a/src/afs/afs_pioctl.c
+++ b/src/afs/afs_pioctl.c
@@ -1790,9 +1790,9 @@ DECL_PIOCTL(PSetTokens)
     struct unixuser *tu;
     struct ClearToken clear;
     struct cell *tcell;
-    char *stp;
+    char *stp, *stpNew;
     char *cellName;
-    int stLen;
+    int stLen, stLenOld;
     struct vrequest treq;
     afs_int32 flag, set_parent_pag = 0;
 
@@ -1866,18 +1866,21 @@ DECL_PIOCTL(PSetTokens)
            areq = &treq;
        }
     }
+
+    stpNew = (char *)afs_osi_Alloc(stLen);
+    if (stpNew == NULL) {
+       return ENOMEM;
+    }
+    memcpy(stpNew, stp, stLen);
+
     /* now we just set the tokens */
     tu = afs_GetUser(areq->uid, i, WRITE_LOCK);        /* i has the cell # */
+    stp = tu->stp;
+    stLenOld = tu->stLen;
+
     tu->vid = clear.ViceId;
-    if (tu->stp != NULL) {
-       afs_osi_Free(tu->stp, tu->stLen);
-    }
-    tu->stp = (char *)afs_osi_Alloc(stLen);
-    if (tu->stp == NULL) {
-       return ENOMEM;
-    }
+    tu->stp = stpNew;
     tu->stLen = stLen;
-    memcpy(tu->stp, stp, stLen);
     tu->ct = clear;
 #ifndef AFS_NOSTATS
     afs_stats_cmfullperf.authent.TicketUpdates++;
@@ -1891,6 +1894,10 @@ DECL_PIOCTL(PSetTokens)
     afs_NotifyUser(tu, UTokensObtained);
     afs_PutUser(tu, WRITE_LOCK);
 
+    if (stp) {
+       afs_osi_Free(stp, stLenOld);
+    }
+
     return 0;
 
   nocell: