rx: Do not ignore RXS_* op errors 22/13522/4
authorAndrew Deason <adeason@sinenomine.net>
Wed, 13 Mar 2019 23:30:43 +0000 (18:30 -0500)
committerBenjamin Kaduk <kaduk@mit.edu>
Sat, 23 Mar 2019 22:43:40 +0000 (18:43 -0400)
Several places in rx call an RXS_* security layer operation, but
ignore the error code. Though errors for these operations are rare or
impossible currently, if they ever do return an error there could be
noticeable consequences, like a connection getting an uninitialized
challenge nonce, or sending a challenge packet with uninitialized
payload.

Change these call sites to record and handle the error. Errors from
the security class normally mean aborting the entire conn, but for
many operations we need to behave differently:

- For RXS_DestroyConnection, errors don't make sense, since we're just
  freeing an object. Change the op to return void, and update our
  implementations of DestroyConnection to match.

- For RXS_GetStats, just clear the relevant stats structure on error
  instead. This change also results in us clearing the stats structure
  when there is no security class associated with the connection;
  previously we just reused the same struct data as the previous conn.

- For RXS_CreateChallenge, aborting the entire conn is difficult,
  because some code paths have callers that potentially lock multiple
  calls on the same conn (rxi_UpdatePeerReach -> TryAttach ->
  rxi_ChallengeOn -> RXS_CreateChallenge), and aborting our conn
  requires locking every call on the conn. So instead we just
  propagate an error up to our callers, and we abort just the call we
  have.

- For RXS_GetChallenge, we cannot abort the conn when
  rxi_ChallengeEvent is called directly, because the caller will have
  the call locked. But when rxi_ChallengeEvent is called as an event
  (when we retry sending the challenge), we can.

- For RXS_SetConfiguration, propagate the error up to our caller.
  Update all rx_SetSecurityConfiguration callers to record and handle
  the error; all of these are during initialization of daemons, so
  have them log an error and exit.

Change-Id: I138b3e06da00470c7d70c458879cc741d296d225
Reviewed-on: https://gerrit.openafs.org/13522
Tested-by: BuildBot <buildbot@rampaginggeek.com>
Reviewed-by: Benjamin Kaduk <kaduk@mit.edu>

12 files changed:
src/bozo/bosserver.c
src/ptserver/ptserver.c
src/rx/rx.c
src/rx/rx.h
src/rx/rx_packet.c
src/rxgk/rxgk_client.c
src/rxgk/rxgk_server.c
src/rxkad/rxkad_common.c
src/rxkad/rxkad_prototypes.h
src/viced/viced.c
src/vlserver/vlserver.c
src/volser/volmain.c

index df38bd4..3a7a6a0 100644 (file)
@@ -1210,8 +1210,12 @@ main(int argc, char **argv, char **envp)
     rx_SetMaxProcs(tservice, 4);
     rx_SetStackSize(tservice, BOZO_LWP_STACKSIZE);     /* so gethostbyname works (in cell stuff) */
     if (rxkadDisableDotCheck) {
-        rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
-                                    (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       code = rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
+                                          (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       if (code) {
+           bozo_Log("Failed to allow dotted principals: code %d\n", code);
+           exit(1);
+       }
     }
 
     tservice =
index d0940fd..34d9819 100644 (file)
@@ -587,8 +587,12 @@ main(int argc, char **argv)
     rx_SetMinProcs(tservice, 2);
     rx_SetMaxProcs(tservice, lwps);
     if (rxkadDisableDotCheck) {
-        rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
-                                    (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       code = rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
+                                          (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       if (code) {
+           afs_com_err(whoami, code, "Failed to allow dotted principals");
+           PT_EXIT(3);
+       }
     }
 
     tservice =
index 61de8c3..2383e5c 100644 (file)
@@ -152,7 +152,7 @@ static void rxi_ScheduleNatKeepAliveEvent(struct rx_connection *conn);
 static void rxi_ScheduleGrowMTUEvent(struct rx_call *call, int secs);
 static void rxi_KeepAliveOn(struct rx_call *call);
 static void rxi_GrowMTUOn(struct rx_call *call);
-static void rxi_ChallengeOn(struct rx_connection *conn);
+static int rxi_ChallengeOn(struct rx_connection *conn);
 static int rxi_CheckCall(struct rx_call *call, int haveCTLock);
 static void rxi_AckAllInTransmitQueue(struct rx_call *call);
 static void rxi_CancelKeepAliveEvent(struct rx_call *call);
@@ -1045,6 +1045,7 @@ rx_NewConnection(afs_uint32 shost, u_short sport, u_short sservice,
 {
     int hashindex, i;
     struct rx_connection *conn;
+    int code;
 
     SPLVAR;
 
@@ -1088,7 +1089,7 @@ rx_NewConnection(afs_uint32 shost, u_short sport, u_short sservice,
        conn->lastBusy[i] = 0;
     }
 
-    RXS_NewConnection(securityObject, conn);
+    code = RXS_NewConnection(securityObject, conn);
     hashindex =
        CONN_HASH(shost, sport, conn->cid, conn->epoch, RX_CLIENT_CONNECTION);
 
@@ -1099,6 +1100,9 @@ rx_NewConnection(afs_uint32 shost, u_short sport, u_short sservice,
        rx_atomic_inc(&rx_stats.nClientConns);
     MUTEX_EXIT(&rx_connHashTable_lock);
     USERPRI;
+    if (code) {
+       rxi_ConnectionError(conn, code);
+    }
     return conn;
 }
 
@@ -1836,10 +1840,14 @@ rx_SetSecurityConfiguration(struct rx_service *service,
                            void *value)
 {
     int i;
+    int code;
     for (i = 0; i<service->nSecurityObjects; i++) {
        if (service->securityObjects[i]) {
-           RXS_SetConfiguration(service->securityObjects[i], NULL, type,
-                                value, NULL);
+           code = RXS_SetConfiguration(service->securityObjects[i], NULL, type,
+                                       value, NULL);
+           if (code) {
+               return code;
+           }
        }
     }
     return 0;
@@ -3113,6 +3121,7 @@ rxi_FindConnection(osi_socket socket, afs_uint32 host,
                    int *unknownService)
 {
     int hashindex, flag, i;
+    int code = 0;
     struct rx_connection *conn;
     *unknownService = 0;
     hashindex = CONN_HASH(host, port, cid, epoch, type);
@@ -3187,7 +3196,7 @@ rxi_FindConnection(osi_socket socket, afs_uint32 host,
            conn->rwind[i] = rx_initReceiveWindow;
        }
        /* Notify security object of the new connection */
-       RXS_NewConnection(conn->securityObject, conn);
+       code = RXS_NewConnection(conn->securityObject, conn);
        /* XXXX Connection timeout? */
        if (service->newConnProc)
            (*service->newConnProc) (conn);
@@ -3199,6 +3208,9 @@ rxi_FindConnection(osi_socket socket, afs_uint32 host,
 
     rxLastConn = conn;         /* store this connection as the last conn used */
     MUTEX_EXIT(&rx_connHashTable_lock);
+    if (code) {
+       rxi_ConnectionError(conn, code);
+    }
     return conn;
 }
 
@@ -3801,7 +3813,7 @@ rxi_CheckConnReach(struct rx_connection *conn, struct rx_call *call)
 static void
 TryAttach(struct rx_call *acall, osi_socket socket,
          int *tnop, struct rx_call **newcallp,
-         int reachOverride)
+         int reachOverride, int istack)
 {
     struct rx_connection *conn = acall->conn;
 
@@ -3815,7 +3827,19 @@ TryAttach(struct rx_call *acall, osi_socket socket,
             * may not any proc available
             */
        } else {
-           rxi_ChallengeOn(acall->conn);
+           int code;
+           code = rxi_ChallengeOn(acall->conn);
+           if (code) {
+               /*
+                * Ideally we would rxi_ConnectionError here, but doing that is
+                * difficult, because some callers may have locked 'call',
+                * _and_ another call on the same conn. So we cannot
+                * rxi_ConnectionError, since that needs to lock every call on
+                * the conn. But we can at least abort the call we have.
+                */
+               rxi_CallError(acall, code);
+               rxi_SendCallAbort(acall, NULL, istack, 0);
+           }
        }
     }
 }
@@ -3975,7 +3999,7 @@ rxi_ReceiveDataPacket(struct rx_call *call,
             * server thread is available, this thread becomes a server
             * thread and the server thread becomes a listener thread. */
            if (isFirst) {
-               TryAttach(call, socket, tnop, newcallp, 0);
+               TryAttach(call, socket, tnop, newcallp, 0, istack);
            }
        }
        /* This is not the expected next packet. */
@@ -4156,7 +4180,8 @@ rxi_ReceiveDataPacket(struct rx_call *call,
 }
 
 static void
-rxi_UpdatePeerReach(struct rx_connection *conn, struct rx_call *acall)
+rxi_UpdatePeerReach(struct rx_connection *conn, struct rx_call *acall,
+                   int istack)
 {
     struct rx_peer *peer = conn->peer;
 
@@ -4177,7 +4202,7 @@ rxi_UpdatePeerReach(struct rx_connection *conn, struct rx_call *acall)
                if (call != acall)
                    MUTEX_ENTER(&call->lock);
                /* tnop can be null if newcallp is null */
-               TryAttach(call, (osi_socket) - 1, NULL, NULL, 1);
+               TryAttach(call, (osi_socket) - 1, NULL, NULL, 1, istack);
                if (call != acall)
                    MUTEX_EXIT(&call->lock);
            }
@@ -4271,7 +4296,7 @@ rxi_ReceiveAckPacket(struct rx_call *call, struct rx_packet *np,
     }
 
     if (ap->reason == RX_ACK_PING_RESPONSE)
-       rxi_UpdatePeerReach(conn, call);
+       rxi_UpdatePeerReach(conn, call, istack);
 
     if (conn->lastPacketSizeSeq) {
        MUTEX_ENTER(&conn->conn_data_lock);
@@ -4815,7 +4840,7 @@ rxi_ReceiveResponsePacket(struct rx_connection *conn,
         * some calls went into attach-wait while we were waiting
         * for authentication..
         */
-       rxi_UpdatePeerReach(conn, NULL);
+       rxi_UpdatePeerReach(conn, NULL, istack);
     }
     return np;
 }
@@ -6236,6 +6261,7 @@ void
 rxi_Send(struct rx_call *call, struct rx_packet *p,
         int istack)
 {
+    int code;
     struct rx_connection *conn = call->conn;
 
     /* Stamp each packet with the user supplied status */
@@ -6243,7 +6269,15 @@ rxi_Send(struct rx_call *call, struct rx_packet *p,
 
     /* Allow the security object controlling this call's security to
      * make any last-minute changes to the packet */
-    RXS_SendPacket(conn->securityObject, call, p);
+    code = RXS_SendPacket(conn->securityObject, call, p);
+    if (code) {
+       MUTEX_EXIT(&call->lock);
+       CALL_HOLD(call, RX_CALL_REFCOUNT_SEND);
+       rxi_ConnectionError(conn, code);
+       CALL_RELE(call, RX_CALL_REFCOUNT_SEND);
+       MUTEX_ENTER(&call->lock);
+       return;
+    }
 
     /* Since we're about to send SOME sort of packet to the peer, it's
      * safe to nuke any scheduled end-of-packets ack */
@@ -6802,12 +6836,28 @@ rxi_ChallengeEvent(struct rxevent *event,
 
        packet = rxi_AllocPacket(RX_PACKET_CLASS_SPECIAL);
        if (packet) {
-           /* If there's no packet available, do this later. */
-           RXS_GetChallenge(conn->securityObject, conn, packet);
-           rxi_SendSpecial((struct rx_call *)0, conn, packet,
-                           RX_PACKET_TYPE_CHALLENGE, NULL, -1, 0);
+           int code;
+           code = RXS_GetChallenge(conn->securityObject, conn, packet);
+           if (code && event_raised) {
+               /*
+                * We can only rxi_ConnectionError the connection if we are
+                * running as an event. Otherwise, the caller may have our call
+                * locked, and so we cannot call rxi_ConnectionError (since it
+                * tries to lock each call in the conn).
+                */
+               rxi_FreePacket(packet);
+               rxi_ConnectionError(conn, code);
+               goto done;
+           }
+           if (code == 0) {
+               /* Only send a challenge packet if we were able to allocate a
+                * packet, and the security layer successfully populated the
+                * challenge. */
+               rxi_SendSpecial((struct rx_call *)0, conn, packet,
+                               RX_PACKET_TYPE_CHALLENGE, NULL, -1, 0);
+               conn->securityChallengeSent = 1;
+           }
            rxi_FreePacket(packet);
-           conn->securityChallengeSent = 1;
        }
        clock_GetTime(&now);
        when = now;
@@ -6832,7 +6882,7 @@ rxi_ChallengeEvent(struct rxevent *event,
  * the call times out, or an invalid response is returned.  The
  * security object associated with the connection is asked to create
  * the challenge at this time. */
-static void
+static int
 rxi_ChallengeOn(struct rx_connection *conn)
 {
     int start = 0;
@@ -6841,9 +6891,14 @@ rxi_ChallengeOn(struct rx_connection *conn)
        start = 1;
     MUTEX_EXIT(&conn->conn_data_lock);
     if (start) {
-       RXS_CreateChallenge(conn->securityObject, conn);
+       int code;
+       code = RXS_CreateChallenge(conn->securityObject, conn);
+       if (code) {
+           return code;
+       }
        rxi_ChallengeEvent(NULL, conn, 0, RX_CHALLENGE_MAXTRIES);
-    };
+    }
+    return 0;
 }
 
 
index 1b3887b..8d5f648 100644 (file)
@@ -588,8 +588,8 @@ struct rx_securityClass {
        int (*op_CheckPacket) (struct rx_securityClass * aobj,
                               struct rx_call * acall,
                               struct rx_packet * apacket);
-       int (*op_DestroyConnection) (struct rx_securityClass * aobj,
-                                    struct rx_connection * aconn);
+       void (*op_DestroyConnection) (struct rx_securityClass * aobj,
+                                     struct rx_connection * aconn);
        int (*op_GetStats) (struct rx_securityClass * aobj,
                            struct rx_connection * aconn,
                            struct rx_securityObjectStats * astats);
@@ -606,6 +606,10 @@ struct rx_securityClass {
 };
 
 #define RXS_OP(obj,op,args) ((obj && (obj->ops->op_ ## op)) ? (*(obj)->ops->op_ ## op)args : 0)
+#define RXS_OP_VOID(obj,op,args) do { \
+    if (obj && (obj->ops->op_ ## op)) \
+       (*(obj)->ops->op_ ## op)args; \
+} while (0)
 
 #define RXS_Close(obj) RXS_OP(obj,Close,(obj))
 #define RXS_NewConnection(obj,conn) RXS_OP(obj,NewConnection,(obj,conn))
@@ -617,7 +621,7 @@ struct rx_securityClass {
 #define RXS_GetResponse(obj,conn,packet) RXS_OP(obj,GetResponse,(obj,conn,packet))
 #define RXS_CheckResponse(obj,conn,packet) RXS_OP(obj,CheckResponse,(obj,conn,packet))
 #define RXS_CheckPacket(obj,call,packet) RXS_OP(obj,CheckPacket,(obj,call,packet))
-#define RXS_DestroyConnection(obj,conn) RXS_OP(obj,DestroyConnection,(obj,conn))
+#define RXS_DestroyConnection(obj,conn) RXS_OP_VOID(obj,DestroyConnection,(obj,conn))
 #define RXS_GetStats(obj,conn,stats) RXS_OP(obj,GetStats,(obj,conn,stats))
 #define RXS_SetConfiguration(obj, conn, type, value, currentValue) RXS_OP(obj, SetConfiguration,(obj,conn,type,value,currentValue))
 
index 859231c..a61665c 100644 (file)
@@ -1874,6 +1874,7 @@ rxi_ReceiveDebugPacket(struct rx_packet *ap, osi_socket asocket,
                for (tc = rx_connHashTable[i]; tc; tc = tc->next) {
                    if ((all || rxi_IsConnInteresting(tc))
                        && tin.index-- <= 0) {
+                       int do_secstats = 0;
                        tconn.host = tc->peer->host;
                        tconn.port = tc->peer->port;
                        tconn.cid = htonl(tc->cid);
@@ -1899,8 +1900,14 @@ rxi_ReceiveDebugPacket(struct rx_packet *ap, osi_socket asocket,
                        tconn.type = tc->type;
                        tconn.securityIndex = tc->securityIndex;
                        if (tc->securityObject) {
-                           RXS_GetStats(tc->securityObject, tc,
-                                        &tconn.secStats);
+                           int code;
+                           code = RXS_GetStats(tc->securityObject, tc,
+                                               &tconn.secStats);
+                           if (code == 0) {
+                               do_secstats = 1;
+                           }
+                       }
+                       if (do_secstats) {
 #define DOHTONL(a) (tconn.secStats.a = htonl(tconn.secStats.a))
 #define DOHTONS(a) (tconn.secStats.a = htons(tconn.secStats.a))
                            DOHTONL(flags);
@@ -1919,6 +1926,8 @@ rxi_ReceiveDebugPacket(struct rx_packet *ap, osi_socket asocket,
                                 sizeof(tconn.secStats.sparel) /
                                 sizeof(afs_int32); i++)
                                DOHTONL(sparel[i]);
+                       } else {
+                           memset(&tconn.secStats, 0, sizeof(tconn.secStats));
                        }
 
                        MUTEX_EXIT(&rx_connHashTable_lock);
index b6b231e..c0305e7 100644 (file)
@@ -58,8 +58,8 @@ static int rxgk_GetResponse(struct rx_securityClass *aobj,
 static int rxgk_ClientCheckPacket(struct rx_securityClass *aobj,
                                  struct rx_call *acall,
                                  struct rx_packet *apacket);
-static int rxgk_DestroyClientConnection(struct rx_securityClass *aobj,
-                                       struct rx_connection *aconn);
+static void rxgk_DestroyClientConnection(struct rx_securityClass *aobj,
+                                        struct rx_connection *aconn);
 static int rxgk_ClientGetStats(struct rx_securityClass *aobj,
                               struct rx_connection *aconn,
                               struct rx_securityObjectStats *astats);
@@ -129,11 +129,10 @@ rxgk_ClientCheckPacket(struct rx_securityClass *aobj, struct rx_call *acall,
     return RXGK_INCONSISTENCY;
 }
 
-static int
+static void
 rxgk_DestroyClientConnection(struct rx_securityClass *aobj,
                             struct rx_connection *aconn)
 {
-    return RXGK_INCONSISTENCY;
 }
 
 static int
index 5fde392..241b8c8 100644 (file)
@@ -65,8 +65,8 @@ static int rxgk_CheckResponse(struct rx_securityClass *aobj,
                              struct rx_packet *apacket);
 static int rxgk_ServerCheckPacket(struct rx_securityClass *aobj,
                                  struct rx_call *acall, struct rx_packet *apacket);
-static int rxgk_DestroyServerConnection(struct rx_securityClass *aobj,
-                                       struct rx_connection *aconn);
+static void rxgk_DestroyServerConnection(struct rx_securityClass *aobj,
+                                        struct rx_connection *aconn);
 static int rxgk_ServerGetStats(struct rx_securityClass *aobj,
                               struct rx_connection *aconn,
                               struct rx_securityObjectStats *astats);
@@ -156,11 +156,10 @@ rxgk_ServerCheckPacket(struct rx_securityClass *aobj, struct rx_call *acall,
     return RXGK_INCONSISTENCY;
 }
 
-static int
+static void
 rxgk_DestroyServerConnection(struct rx_securityClass *aobj,
                             struct rx_connection *aconn)
 {
-    return RXGK_INCONSISTENCY;
 }
 
 static int
index 0916854..5f6ba9b 100644 (file)
@@ -364,7 +364,7 @@ rxkad_NewConnection(struct rx_securityClass *aobj,
 
 /* either: called to destroy a connection. */
 
-int
+void
 rxkad_DestroyConnection(struct rx_securityClass *aobj,
                        struct rx_connection *aconn)
 {
@@ -391,7 +391,7 @@ rxkad_DestroyConnection(struct rx_securityClass *aobj,
        cconn = rx_GetSecurityData(aconn);
        tcp = (struct rxkad_cprivate *)aobj->privateData;
        if (!(tcp->type & rxkad_client))
-           return RXKADINCONSISTENCY;
+           return; /* something is weird; bail out */
        if (cconn) {
            rx_SetSecurityData(aconn, NULL);
            rxi_Free(cconn, sizeof(struct rxkad_cconn));
@@ -400,12 +400,8 @@ rxkad_DestroyConnection(struct rx_securityClass *aobj,
     }
     aobj->refCount--;          /* decrement connection counter */
     if (aobj->refCount <= 0) {
-       afs_int32 code;
-       code = FreeObject(aobj);
-       if (code)
-           return code;
+       (void)FreeObject(aobj);
     }
-    return 0;
 }
 
 /* either: decode packet */
index a13c9fe..e7774a8 100644 (file)
@@ -66,8 +66,8 @@ extern void rxkad_SetLevel(struct rx_connection *conn, rxkad_level level);
 extern int rxkad_Close(struct rx_securityClass *aobj);
 extern int rxkad_NewConnection(struct rx_securityClass *aobj,
                               struct rx_connection *aconn);
-extern int rxkad_DestroyConnection(struct rx_securityClass *aobj,
-                                  struct rx_connection *aconn);
+extern void rxkad_DestroyConnection(struct rx_securityClass *aobj,
+                                   struct rx_connection *aconn);
 extern int rxkad_CheckPacket(struct rx_securityClass *aobj,
                             struct rx_call *acall,
                             struct rx_packet *apacket);
index e301eb9..55c35df 100644 (file)
@@ -2037,8 +2037,12 @@ main(int argc, char *argv[])
        exit(-1);
     }
     if (rxkadDisableDotCheck) {
-        rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
-                                    (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       code = rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
+                                          (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       if (code) {
+           ViceLog(0, ("Failed to allow dotted principals: code %d\n", code));
+           exit(-1);
+       }
     }
     rx_SetMinProcs(tservice, 3);
     rx_SetMaxProcs(tservice, lwps);
index 0c27056..5e07b7a 100644 (file)
@@ -520,8 +520,13 @@ main(int argc, char **argv)
     rx_SetMaxProcs(tservice, lwps);
 
     if (rxkadDisableDotCheck) {
-        rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
-                                    (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       code = rx_SetSecurityConfiguration(tservice, RXS_CONFIG_FLAGS,
+                                          (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       if (code) {
+           VLog(0, ("vlserver: failed to allow dotted principals: %s\n",
+                afs_error_message(code)));
+           exit(2);
+       }
     }
 
     tservice =
index b4a5ee9..ed69bd2 100644 (file)
@@ -619,8 +619,14 @@ main(int argc, char **argv)
 #endif
 
     if (rxkadDisableDotCheck) {
-        rx_SetSecurityConfiguration(service, RXS_CONFIG_FLAGS,
-                                    (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       code = rx_SetSecurityConfiguration(service, RXS_CONFIG_FLAGS,
+                                          (void *)RXS_CONFIG_FLAGS_DISABLE_DOTCHECK);
+       if (code) {
+           fprintf(stderr,
+                   "volser: failed to allow dotted principals: code %d\n",
+                   code);
+           VS_EXIT(1);
+       }
     }
 
     service =