osdc: Objecter::linger_by_cookie() for safe cast from uint64

a `linger_ops_set` was added for `Objecter::handle_watch_notify()`
as a safety check before casting `uint64_t cookie` to `LingerOp*`
and deferencing it

neorados also made use of this set through `Objecter::is_valid_watch()`
checks. however, this approach was still susceptible to use-after-free,
because the callers didn't preserve a LingerOp reference between this
check and its use - and the Objecter lock is dropped in between. in
addition, `neorados::RADOS::unwatch_()` was missing its check for
`is_valid_watch()`

librados did not make use of this `is_valid_watch()` at all, so was
casting cookies directly to LingerOp* and dereferencing. this results
in use-after-free for any cookies invalidated by `linger_cancel()` -
for example when called by `CB_DoWatchError`

replace `is_valid_watch()` with a `linger_by_cookie()` function that
* performs the validity check with `linger_ops_set`,
* safely reinterpret_casts the cookie to LingerOp*, and
* returns a reference to the caller via intrusive_ptr<LingerOp>

`librados::IoCtxImpl::watch_check()`, `unwatch()` and `aio_unwatch()`
now call `linger_by_cookie()`, so have to handle the null case by
returning `-ENOTCONN` (this matches neorados' existing behavior)

Fixes: https://tracker.ceph.com/issues/72771
Signed-off-by: Casey Bodley <cbodley@redhat.com>

diff --git a/src/librados/IoCtxImpl.cc b/src/librados/IoCtxImpl.cc
index 02ef3022274552a346894507e07946f61e130934..ae93b72f480bbf9d459decc643b3fd3c1f4ce751 100644 (file)
--- a/src/librados/IoCtxImpl.cc
+++ b/src/librados/IoCtxImpl.cc
@@ -1745,8 +1745,11 @@ int librados::IoCtxImpl::notify_ack(
 
 int librados::IoCtxImpl::watch_check(uint64_t cookie)
 {
-  auto linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
-  auto r = objecter->linger_check(linger_op);
+  boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
+  if (!linger_op) {
+    return -ENOTCONN;
+  }
+  auto r = objecter->linger_check(linger_op.get());
   if (r)
     return 1 + std::chrono::duration_cast<
       std::chrono::milliseconds>(*r).count();
@@ -1756,7 +1759,11 @@ int librados::IoCtxImpl::watch_check(uint64_t cookie)
 
 int librados::IoCtxImpl::unwatch(uint64_t cookie)
 {
-  Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
+  boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
+  if (!linger_op) {
+    return -ENOTCONN;
+  }
+
   C_SaferCond onfinish;
   version_t ver = 0;
 
@@ -1766,7 +1773,7 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie)
   objecter->mutate(linger_op->target.base_oid, oloc, wr,
                   snapc, ceph::real_clock::now(), extra_op_flags,
                   &onfinish, &ver);
-  objecter->linger_cancel(linger_op);
+  objecter->linger_cancel(linger_op.get());
 
   int r = onfinish.wait();
   set_sync_op_version(ver);
@@ -1776,7 +1783,10 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie)
 int librados::IoCtxImpl::aio_unwatch(uint64_t cookie, AioCompletionImpl *c)
 {
   c->io = this;
-  Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
+  boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
+  if (!linger_op) {
+    return -ENOTCONN;
+  }
   Context *oncomplete = new C_aio_linger_Complete(c, linger_op, true);
 
   ::ObjectOperation wr;

diff --git a/src/neorados/RADOS.cc b/src/neorados/RADOS.cc
index 4dcba611907fb5e67e9295794665d50b475c6610..53b1b42a24ceedaa75f1a2ee4d4d8f58c3d83c26 100644 (file)
--- a/src/neorados/RADOS.cc
+++ b/src/neorados/RADOS.cc
@@ -1570,8 +1570,8 @@ void RADOS::watch_(Object o, IOContext _ioc, WatchComp c,
 }
 
 void RADOS::next_notification_(uint64_t cookie, NextNotificationComp c) {
-  Objecter::LingerOp* linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
-  if (!impl->objecter->is_valid_watch(linger_op)) {
+  boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
+  if (!linger_op) {
     dispatch(asio::append(std::move(c),
                          bs::error_code(ENOTCONN, bs::generic_category()),
                          Notification{}));
@@ -1611,9 +1611,9 @@ void RADOS::notify_ack_(Object o, IOContext _ioc,
 
 tl::expected<ceph::timespan, bs::error_code> RADOS::check_watch(uint64_t cookie)
 {
-  auto linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
-  if (impl->objecter->is_valid_watch(linger_op)) {
-    return impl->objecter->linger_check(linger_op);
+  boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
+  if (linger_op) {
+    return impl->objecter->linger_check(linger_op.get());
   } else {
     return tl::unexpected(bs::error_code(ENOTCONN, bs::generic_category()));
   }
@@ -1624,7 +1624,12 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc,
 {
   auto ioc = reinterpret_cast<const IOContextImpl*>(&_ioc.impl);
 
-  Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
+  boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
+  if (!linger_op) {
+    dispatch(asio::append(std::move(c),
+                         bs::error_code(ENOTCONN, bs::generic_category())));
+    return;
+  }
 
   ObjectOperation op;
   op.watch(cookie, CEPH_OSD_WATCH_OP_UNWATCH);
@@ -1637,7 +1642,7 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc,
                           [objecter = impl->objecter,
                            linger_op, c = std::move(c)]
                           (bs::error_code ec) mutable {
-                            objecter->linger_cancel(linger_op);
+                            objecter->linger_cancel(linger_op.get());
                             asio::dispatch(asio::append(std::move(c), ec));
                           }));
 }

diff --git a/src/osdc/Objecter.cc b/src/osdc/Objecter.cc
index 6ccaf9a76aa6a02aaa4bd42f5a405a5331c94b1f..f062d69b8302abc3e076fe6ec8a9783771757402 100644 (file)
--- a/src/osdc/Objecter.cc
+++ b/src/osdc/Objecter.cc
@@ -657,9 +657,10 @@ class CB_DoWatchError {
   boost::intrusive_ptr<Objecter::LingerOp> info;
   bs::error_code ec;
 public:
-  CB_DoWatchError(Objecter *o, Objecter::LingerOp *i,
+  CB_DoWatchError(Objecter *o,
+                 boost::intrusive_ptr<Objecter::LingerOp> i,
                  bs::error_code ec)
-    : objecter(o), info(i), ec(ec) {
+    : objecter(o), info(std::move(i)), ec(ec) {
     info->_queued_async();
   }
   void operator()() {
@@ -812,7 +813,22 @@ void Objecter::_linger_cancel(LingerOp *info)
   }
 }
 
+auto Objecter::linger_by_cookie(uint64_t cookie)
+  -> boost::intrusive_ptr<LingerOp>
+{
+  auto lock = std::shared_lock{rwlock};
+  return _linger_by_cookie(cookie);
+}
 
+auto Objecter::_linger_by_cookie(uint64_t cookie)
+  -> boost::intrusive_ptr<LingerOp>
+{
+  auto info = reinterpret_cast<LingerOp*>(cookie);
+  if (!linger_ops_set.contains(info)) {
+    return nullptr; // invalid or canceled op
+  }
+  return info;
+}
 
 Objecter::LingerOp *Objecter::linger_register(const object_t& oid,
                                              const object_locator_t& oloc,
@@ -919,8 +935,10 @@ struct CB_DoWatchNotify {
   Objecter *objecter;
   boost::intrusive_ptr<Objecter::LingerOp> info;
   boost::intrusive_ptr<MWatchNotify> msg;
-  CB_DoWatchNotify(Objecter *o, Objecter::LingerOp *i, MWatchNotify *m)
-    : objecter(o), info(i), msg(m) {
+  CB_DoWatchNotify(Objecter *o,
+                   boost::intrusive_ptr<Objecter::LingerOp> i,
+                   MWatchNotify *m)
+    : objecter(o), info(std::move(i)), msg(m) {
     info->_queued_async();
   }
   void operator()() {
@@ -935,8 +953,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
     return;
   }
 
-  LingerOp *info = reinterpret_cast<LingerOp*>(m->cookie);
-  if (linger_ops_set.count(info) == 0) {
+  boost::intrusive_ptr info = _linger_by_cookie(m->cookie);
+  if (!info) {
     ldout(cct, 7) << __func__ << " cookie " << m->cookie << " dne" << dendl;
     return;
   }
@@ -945,8 +963,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
     if (!info->last_error) {
       info->last_error = bs::error_code(ENOTCONN, osd_category());
       if (info->handle) {
-       asio::defer(finish_strand, CB_DoWatchError(this, info,
-                                                         info->last_error));
+       boost::system::error_code ec = info->last_error;
+       asio::defer(finish_strand, CB_DoWatchError(this, std::move(info), ec));
       }
     }
   } else if (!info->is_watch) {
@@ -967,7 +985,7 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
       info->on_notify_finish = nullptr;
     }
   } else {
-    asio::defer(finish_strand, CB_DoWatchNotify(this, info, m));
+    asio::defer(finish_strand, CB_DoWatchNotify(this, std::move(info), m));
   }
 }
 

diff --git a/src/osdc/Objecter.h b/src/osdc/Objecter.h
index 51f1687fc49320dc96c2d765d612eb323b19e570..b3ab73281fe12fb315d125deabb72e7978e1853f 100644 (file)
--- a/src/osdc/Objecter.h
+++ b/src/osdc/Objecter.h
@@ -2592,11 +2592,6 @@ public:
   friend class CB_DoWatchError;
 public:
 
-  bool is_valid_watch(LingerOp* op) {
-    std::shared_lock l(rwlock);
-    return linger_ops_set.contains(op);
-  }
-
   template<typename CT>
   auto linger_callback_flush(CT&& ct) {
     auto consigned = boost::asio::consign(
@@ -3253,6 +3248,14 @@ public:
   void linger_cancel(LingerOp *info);  // releases a reference
   void _linger_cancel(LingerOp *info);
 
+  // return the LingerOp associated with the given cookie.
+  // may return nullptr if the cookie is no longer valid
+  boost::intrusive_ptr<LingerOp> linger_by_cookie(uint64_t cookie);
+ private:
+  // internal version that expects the caller to hold rwlock
+  boost::intrusive_ptr<LingerOp> _linger_by_cookie(uint64_t cookie);
+ public:
+
   void _do_watch_notify(boost::intrusive_ptr<LingerOp> info,
                         boost::intrusive_ptr<MWatchNotify> m);