pipe: only read AuthSessionHandler under pipe_lock

session_security, the AuthSessionHandler for a Pipe, is deleted and
recreated while the pipe_lock is held. read_message() is called
without pipe_lock held, and examines session_security. To make this
safe, make session_security a shared_ptr and take a reference to it
while the pipe_lock is still held, and use that shared_ptr in
read_message().

This may have caused crashes like:

*** Error in `qemu-system-x86_64': invalid fastbin entry (free): 0x00007f42a4002de0 ***
======= Backtrace: =========
/lib/x86_64-linux-gnu/libc.so.6(+0x80a46)[0x7f452f1f3a46]
/usr/lib/x86_64-linux-gnu/libnss3.so(PK11_FreeSymKey+0xa8)[0x7f452e72ff98]
/usr/lib/librados.so.2(+0x2a18cd)[0x7f453451a8cd]
/usr/lib/librados.so.2(_ZNK9CryptoKey7encryptEP11CephContextRKN4ceph6buffer4listERS4_RSs+0x71)[0x7f4534519421]
/usr/lib/librados.so.2(_Z21encode_encrypt_enc_blIN4ceph6buffer4listEEvP11CephContextRKT_RK9CryptoKeyRS2_RSs+0xfe)[0x7f453451859e]
/usr/lib/librados.so.2(_Z14encode_encryptIN4ceph6buffer4listEEiP11CephContextRKT_RK9CryptoKeyRS2_RSs+0xa2)[0x7f45345186d2]
/usr/lib/librados.so.2(_ZN19CephxSessionHandler23check_message_signatureEP7Message+0x246)[0x7f4534516866]
/usr/lib/librados.so.2(_ZN4Pipe12read_messageEPP7Message+0xdcc)[0x7f453462ecbc]
/usr/lib/librados.so.2(_ZN4Pipe6readerEv+0xa5c)[0x7f453464059c]
/usr/lib/librados.so.2(_ZN4Pipe6Reader5entryEv+0xd)[0x7f4534643ecd]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x7f8e)[0x7f452f543f8e]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7f452f26da0d]

Partially-fixes: #6480
Backport: dumpling, emperor
Signed-off-by: Josh Durgin <josh.durgin@inktank.com>

diff --git a/src/msg/Pipe.cc b/src/msg/Pipe.cc
index 3d86789a54be98628b26a7cf95b8c83509470d9d..578848aafdfa426548065c70855384862da4b17f 100644 (file)
--- a/src/msg/Pipe.cc
+++ b/src/msg/Pipe.cc
@@ -81,7 +81,6 @@ Pipe::Pipe(SimpleMessenger *r, int st, Connection *con)
     peer_type(-1),
     pipe_lock("SimpleMessenger::Pipe::pipe_lock"),
     state(st),
-    session_security(NULL),
     connection_state(NULL),
     reader_running(false), reader_needs_join(false),
     writer_running(false),
@@ -113,7 +112,6 @@ Pipe::~Pipe()
 {
   assert(out_q.empty());
   assert(sent.empty());
-  delete session_security;
   delete delay_thread;
 }
 
@@ -419,8 +417,7 @@ int Pipe::accept()
       if (state != STATE_ACCEPTING)
        goto shutting_down_msgr_unlocked;
       reply.tag = CEPH_MSGR_TAG_BADAUTHORIZER;
-      delete session_security;
-      session_security = NULL;
+      session_security.reset();
       goto reply;
     } 
 
@@ -658,9 +655,11 @@ int Pipe::accept()
   connection_state->set_features((uint64_t)reply.features & (uint64_t)connect.features);
   ldout(msgr->cct,10) << "accept features " << connection_state->get_features() << dendl;
 
-  delete session_security;
-  session_security = get_auth_session_handler(msgr->cct, connect.authorizer_protocol, session_key,
-                                             connection_state->get_features());
+  session_security.reset(
+      get_auth_session_handler(msgr->cct,
+                              connect.authorizer_protocol,
+                              session_key,
+                              connection_state->get_features()));
 
   // notify
   msgr->dispatch_queue.queue_accept(connection_state.get());
@@ -1099,13 +1098,15 @@ int Pipe::connect()
       // If we have an authorizer, get a new AuthSessionHandler to deal with ongoing security of the
       // connection.  PLR
 
-      delete session_security;
       if (authorizer != NULL) {
-        session_security = get_auth_session_handler(msgr->cct, authorizer->protocol, authorizer->session_key,
-                                                   connection_state->get_features());
+       session_security.reset(
+            get_auth_session_handler(msgr->cct,
+                                    authorizer->protocol,
+                                    authorizer->session_key,
+                                    connection_state->get_features()));
       }  else {
         // We have no authorizer, so we shouldn't be applying security to messages in this pipe.  PLR
-       session_security = NULL;
+       session_security.reset();
       }
 
       msgr->dispatch_queue.queue_connect(connection_state.get());
@@ -1404,6 +1405,9 @@ void Pipe::reader()
       continue;
     }
 
+    // get a reference to the AuthSessionHandler while we have the pipe_lock
+    ceph::shared_ptr<AuthSessionHandler> auth_handler = session_security;
+
     pipe_lock.Unlock();
 
     char buf[80];
@@ -1471,7 +1475,7 @@ void Pipe::reader()
     else if (tag == CEPH_MSGR_TAG_MSG) {
       ldout(msgr->cct,20) << "reader got MSG" << dendl;
       Message *m = 0;
-      int r = read_message(&m);
+      int r = read_message(&m, auth_handler.get());
 
       pipe_lock.Lock();
       
@@ -1680,7 +1684,7 @@ void Pipe::writer()
        // security set up.  Some session security options do not
        // actually calculate and check the signature, but they should
        // handle the calls to sign_message and check_signature.  PLR
-       if (session_security == NULL) {
+       if (session_security.get() == NULL) {
          ldout(msgr->cct, 20) << "writer no session security" << dendl;
        } else {
          if (session_security->sign_message(m)) {
@@ -1766,7 +1770,7 @@ static void alloc_aligned_buffer(bufferlist& data, unsigned len, unsigned off)
   }
 }
 
-int Pipe::read_message(Message **pm)
+int Pipe::read_message(Message **pm, AuthSessionHandler* auth_handler)
 {
   int ret = -1;
   // envelope
@@ -1954,10 +1958,10 @@ int Pipe::read_message(Message **pm)
   //  Check the signature if one should be present.  A zero return indicates success. PLR
   //
 
-  if (session_security == NULL) {
+  if (auth_handler == NULL) {
     ldout(msgr->cct, 10) << "No session security set" << dendl;
   } else {
-    if (session_security->check_message_signature(message)) {
+    if (auth_handler->check_message_signature(message)) {
       ldout(msgr->cct, 0) << "Signature check failed" << dendl;
       ret = -EINVAL;
       goto out_dethrottle;

diff --git a/src/msg/Pipe.h b/src/msg/Pipe.h
index 29d7958ecf80d71ceaeb5bdde5b35dfc627e5164..468a6a52f2c013b98cb05f3efa7bad6971e28ea0 100644 (file)
--- a/src/msg/Pipe.h
+++ b/src/msg/Pipe.h
@@ -15,6 +15,8 @@
 #ifndef CEPH_MSGR_PIPE_H
 #define CEPH_MSGR_PIPE_H
 
+#include "include/memory.h"
+
 #include "msg_types.h"
 #include "Messenger.h"
 #include "auth/AuthSessionHandler.h"
@@ -146,7 +148,7 @@ class DispatchQueue;
 
     // session_security handles any signatures or encryptions required for this pipe's msgs. PLR
 
-    AuthSessionHandler *session_security;
+    ceph::shared_ptr<AuthSessionHandler> session_security;
 
   protected:
     friend class SimpleMessenger;
@@ -181,7 +183,8 @@ class DispatchQueue;
 
     int randomize_out_seq();
 
-    int read_message(Message **pm);
+    int read_message(Message **pm,
+                    AuthSessionHandler *session_security_copy);
     int write_message(ceph_msg_header& h, ceph_msg_footer& f, bufferlist& body);
     /**
      * Write the given data (of length len) to the Pipe's socket. This function