Windows: Prevent pioctl races from crashing afsd_service

The SMB redirector will permit two processes to open the
pioctl file at the same time without giving SMB server an
opportunity to say 'no'.  As a result multiple reads and writes
on the allocated smb_fid->ioctl can play havoc with the pioctl
state.  Since afsd_service doesn't know the writes and reads
are coming from separate requests there is nothing it can do
to prevent incorrect data going to the wrong process.  However,
it can (and should) protect itself when the state becomes invalid.

Two prevention methods are applied:

 1. add an additional state flag that explicitly indicates
    when the ioctl is in the dataout state

 2. validate the length of data in the ioctl input or
    output buffers before copying it.  If the length
    becomes negative, return a CM_ERROR_INVAL error.

In addition, when the invalid state results in a failure to
to find a matching pioctl function do not return CM_ERROR_BADOP.
CM_ERROR_BADOP can only be returned if the SMB operation is not
supported.  Returning it in response to a ReadFile request will
cause the SMB client to drop the connection.

Finally, fix smb_FindFID to prevent the same 'fid' from being
used for more than one open file.

LICENSE MIT

Reviewed-on: http://gerrit.openafs.org/407
Reviewed-by: Jeffrey Altman <jaltman@openafs.org>
Tested-by: Jeffrey Altman <jaltman@openafs.org>

diff --git a/src/WINNT/afsd/cm_ioctl.h b/src/WINNT/afsd/cm_ioctl.h
index d0827f3cab3b95d1bdeb5b8d72358e6362e11f61..c62422ff50828c9af98cb88359ee7bb145b15a81 100644 (file)
--- a/src/WINNT/afsd/cm_ioctl.h
+++ b/src/WINNT/afsd/cm_ioctl.h
@@ -71,6 +71,7 @@ typedef struct cm_ioctl {
 #define CM_IOCTLFLAG_DATAIN    1       /* reading data from client to server */
 #define CM_IOCTLFLAG_LOGON     2       /* got tokens from integrated logon */
 #define CM_IOCTLFLAG_USEUTF8    4       /* this request is using UTF-8 strings */
+#define CM_IOCTLFLAG_DATAOUT    8       /* sending data from server to client */
 
 
 /* 

diff --git a/src/WINNT/afsd/smb.c b/src/WINNT/afsd/smb.c
index 14fdbb332eb576c626ba3615e25d1ff95d159f36..8a259a497a80bd290ddd1ed87e9b7710d1a5acae 100644 (file)
--- a/src/WINNT/afsd/smb.c
+++ b/src/WINNT/afsd/smb.c
@@ -1418,27 +1418,35 @@ smb_fid_t *smb_FindFID(smb_vc_t *vcp, unsigned short fid, int flags)
     smb_fid_t *fidp;
     int newFid = 0;
         
-    if (fid == 0 && !(flags & SMB_FLAG_CREATE))
-        return NULL;
-
-    lock_ObtainWrite(&smb_rctLock);
-    /* figure out if we need to allocate a new file ID */
     if (fid == 0) {
+        if (!(flags & SMB_FLAG_CREATE))
+            return NULL;
         newFid = 1;
-        fid = vcp->fidCounter;
     }
 
+    lock_ObtainWrite(&smb_rctLock);
+    if (newFid)
+        fid = vcp->fidCounter;
   retry:
+
     for(fidp = vcp->fidsp; fidp; fidp = (smb_fid_t *) osi_QNext(&fidp->q)) {
        if (fidp->refCount == 0 && fidp->deleteOk) {
            fidp->refCount++;
            lock_ReleaseWrite(&smb_rctLock);
            smb_ReleaseFID(fidp);
            lock_ObtainWrite(&smb_rctLock);
+            /*
+             * We dropped the smb_rctLock so the fid value we are using
+             * may now be used by another thread.  Start over with the
+             * current vcp->fidCounter.
+             */
+            if (newFid)
+                fid = vcp->fidCounter;
            goto retry;
        }
         if (fid == fidp->fid) {
             if (newFid) {
+                osi_Log1(smb_logp, "smb_FindFID New Fid Requested.  fid %d found -- retrying ...", fid);
                 fid++;
                 if (fid == 0xFFFF) {
                     osi_Log1(smb_logp,
@@ -1455,6 +1463,12 @@ smb_fid_t *smb_FindFID(smb_vc_t *vcp, unsigned short fid, int flags)
     if (!fidp && (flags & SMB_FLAG_CREATE)) {
         char eventName[MAX_PATH];
         EVENT_HANDLE event;
+
+        if (!newFid)
+            osi_Log1(smb_logp, "smb_FindFID New Fid Not Requested, Fid %d Not Found and CREATE flag set.", fid);
+        else
+            osi_Log1(smb_logp, "smb_FindFID New Fid Requested.  Creating fid %d", fid);
+
         sprintf(eventName,"fid_t event vcp=%d fid=%d", vcp->vcID, fid);
         event = thrd_CreateEvent(NULL, FALSE, TRUE, eventName);
         if ( GetLastError() == ERROR_ALREADY_EXISTS ) {
@@ -8419,7 +8433,7 @@ void smb_DispatchPacket(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp,
             smbp = (smb_t *) inp;
 
             osi_Log5(smb_logp,"Dispatch %s mid 0x%x vcp 0x%p lana %d lsn %d",
-                      opName, smbp->mid, vcp,vcp->lana,vcp->lsn);
+                      opName, smbp->mid, vcp, vcp->lana, vcp->lsn);
             if (inp->inCom == 0x1d) {
                 /* Raw Write */
                 code = smb_ReceiveCoreWriteRaw (vcp, inp, outp, rwcp);
@@ -8427,7 +8441,7 @@ void smb_DispatchPacket(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp,
                 code = (*(dp->procp)) (vcp, inp, outp);
             }   
             osi_Log5(smb_logp,"Dispatch return code 0x%x mid 0x%x vcp 0x%p lana %d lsn %d",
-                      code, smbp->mid, vcp,vcp->lana,vcp->lsn);
+                      code, smbp->mid, vcp, vcp->lana, vcp->lsn);
 
             newTime = GetTickCount();
             osi_Log3(smb_logp, "Dispatch %s mid 0x%x duration %d ms", 

diff --git a/src/WINNT/afsd/smb3.c b/src/WINNT/afsd/smb3.c
index 02fda434408bb5cb6244696067b82011c89a836c..14edcec93d4d3932c20e2eb471dbd733891f4a0a 100644 (file)
--- a/src/WINNT/afsd/smb3.c
+++ b/src/WINNT/afsd/smb3.c
@@ -6772,7 +6772,7 @@ long smb_ReceiveV3WriteX(smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
             osi_Log0(smb_logp, "smb_ReceiveV3WriteX offset requires largefile support");
             /* we shouldn't have received this op if we didn't specify
                largefile support */
-            return CM_ERROR_BADOP;
+            return CM_ERROR_INVAL;
         }
 #endif
     }

diff --git a/src/WINNT/afsd/smb_ioctl.c b/src/WINNT/afsd/smb_ioctl.c
index c2c339af4f811f5477a016fae60f3cad57f8bc69..6492f2c437970b18b1b615b6bf3009070b4e9810 100644 (file)
--- a/src/WINNT/afsd/smb_ioctl.c
+++ b/src/WINNT/afsd/smb_ioctl.c
@@ -129,6 +129,7 @@ smb_IoctlPrepareRead(struct smb_fid *fidp, smb_ioctl_t *ioctlp, cm_user_t *userp
 
     if (ioctlp->ioctl.flags & CM_IOCTLFLAG_DATAIN) {
         ioctlp->ioctl.flags &= ~CM_IOCTLFLAG_DATAIN;
+        ioctlp->ioctl.flags |= CM_IOCTLFLAG_DATAOUT;
 
         /* do the call now, or fail if we didn't get an opcode, or
          * enough of an opcode.
@@ -138,26 +139,31 @@ smb_IoctlPrepareRead(struct smb_fid *fidp, smb_ioctl_t *ioctlp, cm_user_t *userp
         memcpy(&opcode, ioctlp->ioctl.inDatap, sizeof(afs_int32));
         ioctlp->ioctl.inDatap += sizeof(afs_int32);
 
-        osi_Log1(afsd_logp, "Ioctl opcode 0x%x", opcode);
-
+        osi_Log1(afsd_logp, "smb_IoctlPrepareRead opcode 0x%x", opcode);
         /* check for opcode out of bounds */
-        if (opcode < 0 || opcode >= SMB_IOCTL_MAXPROCS)
+        if (opcode < 0 || opcode >= SMB_IOCTL_MAXPROCS) {
+            osi_Log0(afsd_logp, "smb_IoctlPrepareRead - invalid opcode");
             return CM_ERROR_TOOBIG;
+        }
 
         /* check for no such proc */
        procp = smb_ioctlProcsp[opcode];
-        if (procp == NULL) 
-            return CM_ERROR_BADOP;
-
+        if (procp == NULL) {
+            osi_Log0(afsd_logp, "smb_IoctlPrepareRead - unassigned opcode");
+            return CM_ERROR_INVAL;
+        }
         /* otherwise, make the call */
         ioctlp->ioctl.outDatap += sizeof(afs_int32); /* reserve room for return code */
         code = (*procp)(ioctlp, userp);
-
-        osi_Log1(afsd_logp, "Ioctl return code 0x%x", code);
+        osi_Log1(afsd_logp, "smb_IoctlPrepareRead operation returns code 0x%x", code);
 
         /* copy in return code */
         memcpy(ioctlp->ioctl.outAllocp, &code, sizeof(afs_int32));
+    } else if (!(ioctlp->ioctl.flags & CM_IOCTLFLAG_DATAOUT)) {
+        osi_Log0(afsd_logp, "Ioctl invalid state - dataout expected");
+        return CM_ERROR_INVAL;
     }
+
     return 0;
 }
 
@@ -185,6 +191,7 @@ smb_IoctlPrepareWrite(smb_fid_t *fidp, smb_ioctl_t *ioctlp)
         ioctlp->ioctl.inDatap = ioctlp->ioctl.inAllocp;
         ioctlp->ioctl.outDatap = ioctlp->ioctl.outAllocp;
         ioctlp->ioctl.flags |= CM_IOCTLFLAG_DATAIN;
+        ioctlp->ioctl.flags &= ~CM_IOCTLFLAG_DATAOUT;
     }
 }       
 
@@ -219,6 +226,11 @@ smb_IoctlRead(smb_fid_t *fidp, smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *o
     }
 
     leftToCopy = (afs_int32)((iop->ioctl.outDatap - iop->ioctl.outAllocp) - iop->ioctl.outCopied);
+    if (leftToCopy < 0) {
+        osi_Log0(afsd_logp, "smb_IoctlRead leftToCopy went negative");
+        cm_ReleaseUser(userp);
+        return CM_ERROR_INVAL;
+    }
     if (count > leftToCopy)
         count = leftToCopy;
 
@@ -340,7 +352,7 @@ afs_int32
 smb_IoctlV3Read(smb_fid_t *fidp, smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t *outp)
 {
     smb_ioctl_t *iop;
-    long count;
+    unsigned short count;
     afs_int32 code;
     long leftToCopy;
     char *op;
@@ -388,8 +400,13 @@ smb_IoctlV3Read(smb_fid_t *fidp, smb_vc_t *vcp, smb_packet_t *inp, smb_packet_t
     }
 
     leftToCopy = (long)((iop->ioctl.outDatap - iop->ioctl.outAllocp) - iop->ioctl.outCopied);
+    if (leftToCopy < 0) {
+        osi_Log0(afsd_logp, "smb_IoctlV3Read leftToCopy went negative");
+        cm_ReleaseUser(userp);
+        return CM_ERROR_INVAL;
+    }
     if (count > leftToCopy) 
-        count = leftToCopy;
+        count = (unsigned short)leftToCopy;
         
     /* 0 and 1 are reserved for request chaining, were setup by our caller,
      * and will be further filled in after we return.
@@ -472,6 +489,11 @@ smb_IoctlReadRaw(smb_fid_t *fidp, smb_vc_t *vcp, smb_packet_t *inp,
     }
 
     leftToCopy = (long)((iop->ioctl.outDatap - iop->ioctl.outAllocp) - iop->ioctl.outCopied);
+    if (leftToCopy < 0) {
+        osi_Log0(afsd_logp, "smb_IoctlReadRaw leftToCopy went negative");
+        code = CM_ERROR_INVAL;
+        goto done;
+    }
 
     ncbp = outp->ncbp;
     memset((char *)ncbp, 0, sizeof(NCB));