From 1d74170a4c252f35968ccfbec8e432582e92f638 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Tue, 1 Apr 2014 11:37:29 -0700 Subject: [PATCH] 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 --- src/msg/Pipe.cc | 36 ++++++++++++++++++++---------------- src/msg/Pipe.h | 7 +++++-- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/src/msg/Pipe.cc b/src/msg/Pipe.cc index 3d86789a54be9..578848aafdfa4 100644 --- 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 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 29d7958ecf80d..468a6a52f2c01 100644 --- 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 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 -- 2.39.5