Skip to content

Commit 94f42b6

Browse files
cbodleycbodley
committed
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 <[email protected]>
1 parent 2455a71 commit 94f42b6

File tree

4 files changed

+62
-26
lines changed

4 files changed

+62
-26
lines changed

‎src/librados/IoCtxImpl.cc‎

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1745,8 +1745,11 @@ int librados::IoCtxImpl::notify_ack(
17451745

17461746
int librados::IoCtxImpl::watch_check(uint64_t cookie)
17471747
{
1748-
auto linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1749-
auto r = objecter->linger_check(linger_op);
1748+
boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
1749+
if (!linger_op) {
1750+
return -ENOTCONN;
1751+
}
1752+
auto r = objecter->linger_check(linger_op.get());
17501753
if (r)
17511754
return 1 + std::chrono::duration_cast<
17521755
std::chrono::milliseconds>(*r).count();
@@ -1756,7 +1759,11 @@ int librados::IoCtxImpl::watch_check(uint64_t cookie)
17561759

17571760
int librados::IoCtxImpl::unwatch(uint64_t cookie)
17581761
{
1759-
Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1762+
boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
1763+
if (!linger_op) {
1764+
return -ENOTCONN;
1765+
}
1766+
17601767
C_SaferCond onfinish;
17611768
version_t ver = 0;
17621769

@@ -1766,7 +1773,7 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie)
17661773
objecter->mutate(linger_op->target.base_oid, oloc, wr,
17671774
snapc, ceph::real_clock::now(), extra_op_flags,
17681775
&onfinish, &ver);
1769-
objecter->linger_cancel(linger_op);
1776+
objecter->linger_cancel(linger_op.get());
17701777

17711778
int r = onfinish.wait();
17721779
set_sync_op_version(ver);
@@ -1776,7 +1783,10 @@ int librados::IoCtxImpl::unwatch(uint64_t cookie)
17761783
int librados::IoCtxImpl::aio_unwatch(uint64_t cookie, AioCompletionImpl *c)
17771784
{
17781785
c->io = this;
1779-
Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1786+
boost::intrusive_ptr linger_op = objecter->linger_by_cookie(cookie);
1787+
if (!linger_op) {
1788+
return -ENOTCONN;
1789+
}
17801790
Context *oncomplete = new C_aio_linger_Complete(c, linger_op, true);
17811791

17821792
::ObjectOperation wr;

‎src/neorados/RADOS.cc‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1570,8 +1570,8 @@ void RADOS::watch_(Object o, IOContext _ioc, WatchComp c,
15701570
}
15711571

15721572
void RADOS::next_notification_(uint64_t cookie, NextNotificationComp c) {
1573-
Objecter::LingerOp* linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1574-
if (!impl->objecter->is_valid_watch(linger_op)) {
1573+
boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
1574+
if (!linger_op) {
15751575
dispatch(asio::append(std::move(c),
15761576
bs::error_code(ENOTCONN, bs::generic_category()),
15771577
Notification{}));
@@ -1611,9 +1611,9 @@ void RADOS::notify_ack_(Object o, IOContext _ioc,
16111611

16121612
tl::expected<ceph::timespan, bs::error_code> RADOS::check_watch(uint64_t cookie)
16131613
{
1614-
auto linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1615-
if (impl->objecter->is_valid_watch(linger_op)) {
1616-
return impl->objecter->linger_check(linger_op);
1614+
boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
1615+
if (linger_op) {
1616+
return impl->objecter->linger_check(linger_op.get());
16171617
} else {
16181618
return tl::unexpected(bs::error_code(ENOTCONN, bs::generic_category()));
16191619
}
@@ -1624,7 +1624,12 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc,
16241624
{
16251625
auto ioc = reinterpret_cast<const IOContextImpl*>(&_ioc.impl);
16261626

1627-
Objecter::LingerOp *linger_op = reinterpret_cast<Objecter::LingerOp*>(cookie);
1627+
boost::intrusive_ptr linger_op = impl->objecter->linger_by_cookie(cookie);
1628+
if (!linger_op) {
1629+
dispatch(asio::append(std::move(c),
1630+
bs::error_code(ENOTCONN, bs::generic_category())));
1631+
return;
1632+
}
16281633

16291634
ObjectOperation op;
16301635
op.watch(cookie, CEPH_OSD_WATCH_OP_UNWATCH);
@@ -1637,7 +1642,7 @@ void RADOS::unwatch_(uint64_t cookie, IOContext _ioc,
16371642
[objecter = impl->objecter,
16381643
linger_op, c = std::move(c)]
16391644
(bs::error_code ec) mutable {
1640-
objecter->linger_cancel(linger_op);
1645+
objecter->linger_cancel(linger_op.get());
16411646
asio::dispatch(asio::append(std::move(c), ec));
16421647
}));
16431648
}

‎src/osdc/Objecter.cc‎

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -657,9 +657,10 @@ class CB_DoWatchError {
657657
boost::intrusive_ptr<Objecter::LingerOp> info;
658658
bs::error_code ec;
659659
public:
660-
CB_DoWatchError(Objecter *o, Objecter::LingerOp *i,
660+
CB_DoWatchError(Objecter *o,
661+
boost::intrusive_ptr<Objecter::LingerOp> i,
661662
bs::error_code ec)
662-
: objecter(o), info(i), ec(ec) {
663+
: objecter(o), info(std::move(i)), ec(ec) {
663664
info->_queued_async();
664665
}
665666
void operator()() {
@@ -812,7 +813,22 @@ void Objecter::_linger_cancel(LingerOp *info)
812813
}
813814
}
814815

816+
auto Objecter::linger_by_cookie(uint64_t cookie)
817+
-> boost::intrusive_ptr<LingerOp>
818+
{
819+
auto lock = std::shared_lock{rwlock};
820+
return _linger_by_cookie(cookie);
821+
}
815822

823+
auto Objecter::_linger_by_cookie(uint64_t cookie)
824+
-> boost::intrusive_ptr<LingerOp>
825+
{
826+
auto info = reinterpret_cast<LingerOp*>(cookie);
827+
if (!linger_ops_set.contains(info)) {
828+
return nullptr; // invalid or canceled op
829+
}
830+
return info;
831+
}
816832

817833
Objecter::LingerOp *Objecter::linger_register(const object_t& oid,
818834
const object_locator_t& oloc,
@@ -919,8 +935,10 @@ struct CB_DoWatchNotify {
919935
Objecter *objecter;
920936
boost::intrusive_ptr<Objecter::LingerOp> info;
921937
boost::intrusive_ptr<MWatchNotify> msg;
922-
CB_DoWatchNotify(Objecter *o, Objecter::LingerOp *i, MWatchNotify *m)
923-
: objecter(o), info(i), msg(m) {
938+
CB_DoWatchNotify(Objecter *o,
939+
boost::intrusive_ptr<Objecter::LingerOp> i,
940+
MWatchNotify *m)
941+
: objecter(o), info(std::move(i)), msg(m) {
924942
info->_queued_async();
925943
}
926944
void operator()() {
@@ -935,8 +953,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
935953
return;
936954
}
937955

938-
LingerOp *info = reinterpret_cast<LingerOp*>(m->cookie);
939-
if (linger_ops_set.count(info) == 0) {
956+
boost::intrusive_ptr info = _linger_by_cookie(m->cookie);
957+
if (!info) {
940958
ldout(cct, 7) << __func__ << " cookie " << m->cookie << " dne" << dendl;
941959
return;
942960
}
@@ -945,8 +963,8 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
945963
if (!info->last_error) {
946964
info->last_error = bs::error_code(ENOTCONN, osd_category());
947965
if (info->handle) {
948-
asio::defer(finish_strand, CB_DoWatchError(this, info,
949-
info->last_error));
966+
boost::system::error_code ec = info->last_error;
967+
asio::defer(finish_strand, CB_DoWatchError(this, std::move(info), ec));
950968
}
951969
}
952970
} else if (!info->is_watch) {
@@ -967,7 +985,7 @@ void Objecter::handle_watch_notify(MWatchNotify *m)
967985
info->on_notify_finish = nullptr;
968986
}
969987
} else {
970-
asio::defer(finish_strand, CB_DoWatchNotify(this, info, m));
988+
asio::defer(finish_strand, CB_DoWatchNotify(this, std::move(info), m));
971989
}
972990
}
973991

‎src/osdc/Objecter.h‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2592,11 +2592,6 @@ class Objecter : public md_config_obs_t, public Dispatcher {
25922592
friend class CB_DoWatchError;
25932593
public:
25942594

2595-
bool is_valid_watch(LingerOp* op) {
2596-
std::shared_lock l(rwlock);
2597-
return linger_ops_set.contains(op);
2598-
}
2599-
26002595
template<typename CT>
26012596
auto linger_callback_flush(CT&& ct) {
26022597
auto consigned = boost::asio::consign(
@@ -3253,6 +3248,14 @@ class Objecter : public md_config_obs_t, public Dispatcher {
32533248
void linger_cancel(LingerOp *info); // releases a reference
32543249
void _linger_cancel(LingerOp *info);
32553250

3251+
// return the LingerOp associated with the given cookie.
3252+
// may return nullptr if the cookie is no longer valid
3253+
boost::intrusive_ptr<LingerOp> linger_by_cookie(uint64_t cookie);
3254+
private:
3255+
// internal version that expects the caller to hold rwlock
3256+
boost::intrusive_ptr<LingerOp> _linger_by_cookie(uint64_t cookie);
3257+
public:
3258+
32563259
void _do_watch_notify(boost::intrusive_ptr<LingerOp> info,
32573260
boost::intrusive_ptr<MWatchNotify> m);
32583261

0 commit comments

Comments
 (0)