From 27d6032ef6dbaa521176be71758609448db1fca9 Mon Sep 17 00:00:00 2001 From: Josh Durgin Date: Tue, 1 Apr 2014 17:27:01 -0700 Subject: [PATCH] auth: add rwlock to AuthClientHandler to prevent races For cephx, build_authorizer reads a bunch of state (especially the current session_key) which can be updated by the MonClient. With no locks held, Pipe::connect() calls SimpleMessenger::get_authorizer() which ends up calling RadosClient::get_authorizer() and then AuthClientHandler::bulid_authorizer(). This unsafe usage can lead to crashes like: Program terminated with signal 11, Segmentation fault. 0x00007fa0d2ddb7cb in ceph::buffer::ptr::release (this=0x7f987a5e3070) at common/buffer.cc:370 370 common/buffer.cc: No such file or directory. in common/buffer.cc (gdb) bt 0x00007fa0d2ddb7cb in ceph::buffer::ptr::release (this=0x7f987a5e3070) at common/buffer.cc:370 0x00007fa0d2ddec00 in ~ptr (this=0x7f989c03b830) at ./include/buffer.h:171 ceph::buffer::list::rebuild (this=0x7f989c03b830) at common/buffer.cc:817 0x00007fa0d2ddecb9 in ceph::buffer::list::c_str (this=0x7f989c03b830) at common/buffer.cc:1045 0x00007fa0d2ea4dc2 in Pipe::connect (this=0x7fa0c4307340) at msg/Pipe.cc:907 0x00007fa0d2ea7d73 in Pipe::writer (this=0x7fa0c4307340) at msg/Pipe.cc:1518 0x00007fa0d2eb44dd in Pipe::Writer::entry (this=) at msg/Pipe.h:59 0x00007fa0e0f5f9d1 in start_thread (arg=0x7f987a5e4700) at pthread_create.c:301 0x00007fa0de560b6d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115 and Error in `qemu-system-x86_64': invalid fastbin entry (free): 0x00007ff12887ff20 *** ======= Backtrace: ========= /lib/x86_64-linux-gnu/libc.so.6(+0x80a46)[0x7ff3dea1fa46] /usr/lib/librados.so.2(+0x29eb03)[0x7ff3e3d43b03] /usr/lib/librados.so.2(_ZNK9CryptoKey7encryptEP11CephContextRKN4ceph6buffer4listERS4_RSs+0x71)[0x7ff3e3d42661] /usr/lib/librados.so.2(_Z21encode_encrypt_enc_blIN4ceph6buffer4listEEvP11CephContextRKT_RK9CryptoKeyRS2_RSs+0xfe)[0x7ff3e3d417de] /usr/lib/librados.so.2(_Z14encode_encryptIN4ceph6buffer4listEEiP11CephContextRKT_RK9CryptoKeyRS2_RSs+0xa2)[0x7ff3e3d41912] /usr/lib/librados.so.2(_ZN19CephxSessionHandler12sign_messageEP7Message+0x242)[0x7ff3e3d40de2] /usr/lib/librados.so.2(_ZN4Pipe6writerEv+0x92b)[0x7ff3e3e61b2b] /usr/lib/librados.so.2(_ZN4Pipe6Writer5entryEv+0xd)[0x7ff3e3e6c7fd] /lib/x86_64-linux-gnu/libpthread.so.0(+0x7f8e)[0x7ff3ded6ff8e] /lib/x86_64-linux-gnu/libc.so.6(clone+0x6d)[0x7ff3dea99a0d] Fix this by adding an rwlock to AuthClientHandler. A simpler fix would be to move RadosClient::get_authorizer() into the MonClient() under the MonClient lock, but this would not catch all uses of other Authorizer, e.g. for verify_authorizer() and it would serialize independent connection attempts. This mainly matters for cephx, but none and unknown can have the global_id reset as well. Partially-fixes: #6480 Backport: dumpling, emperor Signed-off-by: Josh Durgin (cherry picked from commit 2cc76bcd12d803160e98fa73810de2cb916ef1ff) --- src/auth/AuthClientHandler.h | 17 ++++++----------- src/auth/cephx/CephxClientHandler.cc | 8 ++++++++ src/auth/cephx/CephxClientHandler.h | 2 ++ src/auth/none/AuthNoneClientHandler.h | 6 +++++- src/auth/unknown/AuthUnknownClientHandler.h | 6 +++++- 5 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h index 5d129adc111d9..586479ee3d302 100644 --- a/src/auth/AuthClientHandler.h +++ b/src/auth/AuthClientHandler.h @@ -20,6 +20,7 @@ #include "common/Mutex.h" #include "common/Cond.h" +#include "common/RWLock.h" #include "common/Timer.h" @@ -36,33 +37,27 @@ protected: uint32_t want; uint32_t have; uint32_t need; + RWLock lock; public: AuthClientHandler(CephContext *cct_) - : cct(cct_), global_id(0), want(CEPH_ENTITY_TYPE_AUTH), have(0), need(0) {} + : cct(cct_), global_id(0), want(CEPH_ENTITY_TYPE_AUTH), have(0), need(0), + lock("AuthClientHandler::lock") {} virtual ~AuthClientHandler() {} void init(EntityName& n) { name = n; } void set_want_keys(__u32 keys) { + RWLock::WLocker l(lock); want = keys | CEPH_ENTITY_TYPE_AUTH; validate_tickets(); } void add_want_keys(__u32 keys) { + RWLock::WLocker l(lock); want |= keys; validate_tickets(); } - bool have_keys(__u32 k) { - validate_tickets(); - return (k & have) == have; - } - bool have_keys() { - validate_tickets(); - return (want & have) == have; - } - - virtual int get_protocol() = 0; virtual void reset() = 0; diff --git a/src/auth/cephx/CephxClientHandler.cc b/src/auth/cephx/CephxClientHandler.cc index 2818b7a679abf..8a8f44da69762 100644 --- a/src/auth/cephx/CephxClientHandler.cc +++ b/src/auth/cephx/CephxClientHandler.cc @@ -32,8 +32,12 @@ int CephxClientHandler::build_request(bufferlist& bl) ldout(cct, 10) << "build_request" << dendl; ldout(cct, 10) << "validate_tickets: want=" << want << " need=" << need << " have=" << have << dendl; + + lock.get_write(); validate_tickets(); + lock.put_write(); + RWLock::RLocker l(lock); ldout(cct, 10) << "want=" << want << " need=" << need << " have=" << have << dendl; CephXTicketHandler& ticket_handler = tickets.get_handler(CEPH_ENTITY_TYPE_AUTH); @@ -94,6 +98,7 @@ int CephxClientHandler::build_request(bufferlist& bl) int CephxClientHandler::handle_response(int ret, bufferlist::iterator& indata) { ldout(cct, 10) << "handle_response ret = " << ret << dendl; + RWLock::WLocker l(lock); if (ret < 0) return ret; // hrm! @@ -178,6 +183,7 @@ int CephxClientHandler::handle_response(int ret, bufferlist::iterator& indata) AuthAuthorizer *CephxClientHandler::build_authorizer(uint32_t service_id) { + RWLock::RLocker l(lock); ldout(cct, 10) << "build_authorizer for service " << ceph_entity_type_name(service_id) << dendl; return tickets.build_authorizer(service_id); } @@ -194,11 +200,13 @@ bool CephxClientHandler::build_rotating_request(bufferlist& bl) void CephxClientHandler::validate_tickets() { + // lock should be held for write tickets.validate_tickets(want, have, need); } bool CephxClientHandler::need_tickets() { + RWLock::WLocker l(lock); validate_tickets(); ldout(cct, 20) << "need_tickets: want=" << want << " need=" << need << " have=" << have << dendl; diff --git a/src/auth/cephx/CephxClientHandler.h b/src/auth/cephx/CephxClientHandler.h index bf8108ae39179..e5377094b2749 100644 --- a/src/auth/cephx/CephxClientHandler.h +++ b/src/auth/cephx/CephxClientHandler.h @@ -44,6 +44,7 @@ public: } void reset() { + RWLock::WLocker l(lock); starting = true; server_challenge = 0; } @@ -61,6 +62,7 @@ public: bool need_tickets(); void set_global_id(uint64_t id) { + RWLock::WLocker l(lock); global_id = id; tickets.global_id = id; } diff --git a/src/auth/none/AuthNoneClientHandler.h b/src/auth/none/AuthNoneClientHandler.h index 61c1db93ec984..e2630d9743ea8 100644 --- a/src/auth/none/AuthNoneClientHandler.h +++ b/src/auth/none/AuthNoneClientHandler.h @@ -36,6 +36,7 @@ public: void tick() {} AuthAuthorizer *build_authorizer(uint32_t service_id) { + RWLock::RLocker l(lock); AuthNoneAuthorizer *auth = new AuthNoneAuthorizer(); if (auth) { auth->build_authorizer(cct->_conf->name, global_id); @@ -46,7 +47,10 @@ public: void validate_tickets() { } bool need_tickets() { return false; } - void set_global_id(uint64_t id) { global_id = id; } + void set_global_id(uint64_t id) { + RWLock::WLocker l(lock); + global_id = id; + } }; #endif diff --git a/src/auth/unknown/AuthUnknownClientHandler.h b/src/auth/unknown/AuthUnknownClientHandler.h index b26b382e91fdb..bb523db6585d8 100644 --- a/src/auth/unknown/AuthUnknownClientHandler.h +++ b/src/auth/unknown/AuthUnknownClientHandler.h @@ -36,6 +36,7 @@ public: void tick() {} AuthAuthorizer *build_authorizer(uint32_t service_id) { + RWLock::RLocker l(lock); AuthUnknownAuthorizer *auth = new AuthUnknownAuthorizer(); if (auth) { auth->build_authorizer(cct->_conf->name, global_id); @@ -46,7 +47,10 @@ public: void validate_tickets() { } bool need_tickets() { return false; } - void set_global_id(uint64_t id) { global_id = id; } + void set_global_id(uint64_t id) { + RWLock::WLocker l(lock); + global_id = id; + } }; #endif -- 2.39.5