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=<value optimized out>) 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 <josh.durgin@inktank.com>
(cherry picked from commit 2cc76bcd12d803160e98fa73810de2cb916ef1ff)

diff --git a/src/auth/AuthClientHandler.h b/src/auth/AuthClientHandler.h
index 5d129adc111d93fea4b88ddac9ce72c430e87d92..586479ee3d3020e0531077a527dd4bd7300466e8 100644 (file)
--- 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 2818b7a679abf8c2274ae7e94b34e6040c3532fa..8a8f44da697623411134b03a6860a9e99bcf9c00 100644 (file)
--- 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 bf8108ae3917937ad744ad015112836f2500615d..e5377094b2749a0de1b6315c98271669c70de186 100644 (file)
--- 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 61c1db93ec984ae705b560a93568b9d61273af80..e2630d9743ea8638f77f7b72925d4a22b70e55c9 100644 (file)
--- 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 b26b382e91fdbf584cdf42c9ab98c7f216dc0b1f..bb523db6585d87c1f21ab9573a06fd7a65547de1 100644 (file)
--- 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