crimson: fix stack-use-after-return in ConfigProxy::parse_argv

ConfigProxy::parse_argv used to take std::vector<const char*>& and capture
it by reference in a lambda passed to do_change(), which invokes the lambda
on another shard via invoke_on. By the time the target shard ran the lambda,
the caller had returned and the referenced vector was destroyed.

Fix: take std::vector<std::string> by value and move it into the closure,
so the data outlives the call. Convert to vector<const char*> only on the
target shard.

- update callers to pass vector<string>
- add unittest-seastar-config-parse-argv-uaf to demonstrate the pattern

Signed-off-by: Alexander Indenbaum <aindenba@redhat.com>

diff --git a/src/crimson/common/config_proxy.h b/src/crimson/common/config_proxy.h
index ee4595ad3e2fb036fe4b760d471f568f52c6f5c1..c19cfd201ddf74769a88dc7aaa4702294c58d7ae 100644 (file)
--- a/src/crimson/common/config_proxy.h
+++ b/src/crimson/common/config_proxy.h
@@ -192,17 +192,13 @@ public:
   }
   void show_config(ceph::Formatter* f) const;
 
-  seastar::future<> parse_argv(std::vector<const char*>& argv) {
-    // we could pass whatever is unparsed to seastar, but seastar::app_template
-    // is used for driving the seastar application, and
-    // crimson::common::ConfigProxy is not available until seastar engine is up
-    // and running, so we have to feed the command line args to app_template
-    // first, then pass them to ConfigProxy.
-    return do_change([&argv, this](ConfigValues& values) {
-      get_config().parse_argv(values,
-                             obs_mgr,
-                             argv,
-                             CONF_CMDLINE);
+  seastar::future<> parse_argv(std::vector<std::string> args) {
+    // Take args by value so the lambda can safely be invoked on another shard
+    return do_change([args = std::move(args), this](ConfigValues& values) {
+      std::vector<const char*> argv;
+      argv.reserve(args.size());
+      for (const auto& s : args) argv.push_back(s.c_str());
+      get_config().parse_argv(values, obs_mgr, argv, CONF_CMDLINE);
     });
   }
 

diff --git a/src/crimson/osd/main.cc b/src/crimson/osd/main.cc
index 461a2652ecc7b7d3e73cba57e7c999db35aba499..a65905fa40aa4a84a02aa0c8b563d02337608b3e 100644 (file)
--- a/src/crimson/osd/main.cc
+++ b/src/crimson/osd/main.cc
@@ -98,7 +98,7 @@ int main(int argc, const char* argv[])
   auto &early_config = early_config_result.value();
 
   auto seastar_n_early_args = early_config.get_early_args();
-  auto config_proxy_args = early_config.get_ceph_args();
+  auto config_proxy_args = early_config.ceph_args;
 
   seastar::app_template::config app_cfg;
   app_cfg.name = "Crimson";

diff --git a/src/crimson/osd/main_config_bootstrap_helpers.cc b/src/crimson/osd/main_config_bootstrap_helpers.cc
index 090c54ee495da59b5602ca7723330d77f3b12bf6..adf7d630b1392f589c5d5894bc6a91e2b1ba8066 100644 (file)
--- a/src/crimson/osd/main_config_bootstrap_helpers.cc
+++ b/src/crimson/osd/main_config_bootstrap_helpers.cc
@@ -165,7 +165,10 @@ _get_early_config(int argc, const char *argv[])
        auto stop_perf_coll = seastar::deferred_stop(sharded_perf_coll());
 
        local_conf().parse_env().get();
-       local_conf().parse_argv(early_args).get();
+       {
+         std::vector<std::string> args_copy(early_args.begin(), early_args.end());
+         local_conf().parse_argv(std::move(args_copy)).get();
+       }
        local_conf().parse_config_files(ret.conf_file_list).get();
 
        if (local_conf()->no_mon_config) {

diff --git a/src/crimson/tools/store_bench/store-bench.cc b/src/crimson/tools/store_bench/store-bench.cc
index 18fe9af9d5f429bddc3f54d01b888e8135e7dc4d..4811d859e85d22942b1e7164db7c5485d34629a1 100644 (file)
--- a/src/crimson/tools/store_bench/store-bench.cc
+++ b/src/crimson/tools/store_bench/store-bench.cc
@@ -883,11 +883,7 @@ int main(int argc, char **argv) {
         co_await crimson::common::local_conf().start();
 
         {
-          std::vector<const char *> cav;
-          std::transform(
-              std::begin(unrecognized_options), std::end(unrecognized_options),
-              std::back_inserter(cav), [](auto &s) { return s.c_str(); });
-          co_await crimson::common::local_conf().parse_argv(cav);
+          co_await crimson::common::local_conf().parse_argv(unrecognized_options);
         }
 
         auto store = crimson::os::FuturizedStore::create(

diff --git a/src/test/crimson/CMakeLists.txt b/src/test/crimson/CMakeLists.txt
index 48484410a064307e3622ff18065b33490bd50ad6..43028e8c92a4c8491875e90b627d7d4485ccc5fd 100644 (file)
--- a/src/test/crimson/CMakeLists.txt
+++ b/src/test/crimson/CMakeLists.txt
@@ -55,6 +55,12 @@ add_ceph_unittest(unittest-seastar-config
   --memory 256M --smp 4)
 target_link_libraries(unittest-seastar-config crimson)
 
+add_executable(unittest-seastar-config-parse-argv-uaf
+  test_config_parse_argv_uaf.cc)
+add_ceph_unittest(unittest-seastar-config-parse-argv-uaf
+  --memory 256M --smp 2)
+target_link_libraries(unittest-seastar-config-parse-argv-uaf crimson)
+
 add_executable(unittest-seastar-monc
   test_monc.cc)
 target_link_libraries(unittest-seastar-monc crimson)

diff --git a/src/test/crimson/test_config_parse_argv_uaf.cc b/src/test/crimson/test_config_parse_argv_uaf.cc
new file mode 100644 (file)
index 0000000..8ac2460
--- /dev/null
+++ b/src/test/crimson/test_config_parse_argv_uaf.cc
@@ -0,0 +1,93 @@
+// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:nil -*-
+// vim: ts=8 sw=2 expandtab
+//
+// Minimal unit test demonstrating stack-use-after-return when passing
+// a reference to a local variable into a lambda that runs on another shard
+// via invoke_on to replicate the pattern that ConfigProxy::parse_argv had before
+// the fix (taking vector<const char*>& and capturing by reference in do_change)
+//
+// The old path captures [&argv]; the lambda runs on shard 1 after the caller
+// on shard 0 has returned, so argv (on shard 0's stack) is destroyed
+//
+// Run: ./bin/unittest-seastar-config-parse-argv-uaf --smp 2
+// Without ASan stack-use-after-return: may crash (SIGABRT) due to UAF
+// With ASAN_OPTIONS=detect_stack_use_after_return=1: may detect the UAF
+//
+// Build: ninja unittest-seastar-config-parse-argv-uaf
+//
+
+#include <seastar/core/app-template.hh>
+#include <seastar/core/sharded.hh>
+#include <seastar/core/smp.hh>
+#include "test/crimson/ctest_utils.h"
+
+using namespace seastar;
+
+struct TargetService {};
+
+static sharded<TargetService>* g_svc = nullptr;
+
+// Mimics the old ConfigProxy::parse_argv pattern: takes reference, captures
+// in lambda sent to another shard. When lambda runs, caller's stack is gone.
+static future<> buggy_parse_argv_like(std::vector<const char*>& argv) {
+  return g_svc->invoke_on(1, [&argv](TargetService&) {
+    volatile size_t n = argv.size();  // use-after-return: argv was on caller's stack
+    (void)n;
+  });
+}
+
+// New approach: take by value, move into closure so data outlives the call
+static future<> correct_parse_argv_like(std::vector<std::string> args) {
+  return g_svc->invoke_on(1, [args = std::move(args)](TargetService&) {
+    volatile size_t n = args.size();  // safe: args is captured by value
+    (void)n;
+  });
+}
+
+static future<> test_uaf()
+{
+  if (smp::count < 2) {
+    std::cerr << "Skip: need --smp 2 to demonstrate cross-shard use-after-return" << std::endl;
+    return make_ready_future<>();
+  }
+
+  // Run the old call from shard 0. The lambda is sent to shard 1.
+  // By the time shard 1 runs it, the caller on shard 0 has returned and
+  // 'argv' (on shard 0's stack) is gone.
+  return smp::submit_to(0, [] {
+    std::vector<const char*> argv = {"--foo=1", "--bar=2"};
+    return buggy_parse_argv_like(argv);
+  });
+}
+
+static future<> test_correct()
+{
+  if (smp::count < 2) {
+    return make_ready_future<>();
+  }
+  std::vector<std::string> args = {"--foo=1", "--bar=2"};
+  return correct_parse_argv_like(std::move(args));
+}
+
+int main(int argc, char** argv)
+{
+  app_template app{get_smp_opts_from_ctest()};
+  static sharded<TargetService> svc;  // static so it outlives the async chain
+  return app.run(argc, argv, [&] {
+    return svc.start().then([] {
+      g_svc = &svc;
+      return test_correct();
+    }).then([] {
+      return test_uaf();
+    }).finally([] {
+      g_svc = nullptr;
+      return svc.stop();
+    }).then([] {
+      std::cout << "Done. With ASAN_OPTIONS=detect_stack_use_after_return=1, "
+                << "the buggy path may trigger use-after-return detection." << std::endl;
+    }).handle_exception([](auto e) {
+      std::cerr << "Test failed: " << e << std::endl;
+      return make_exception_future<>(e);
+    });
+  });
+}