From ab8865b182712df5bf5dbb1afdfba8d7ff5b7111 Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:41:42 -0700 Subject: [PATCH] fix: enforce regmap size invariant to close OOB writes (#64, #65) Ep0's per-endpoint access checks (check_read_access, check_write_access, set_endpoint_owner) all bound a target endpoint against endpoint_owner_, which is sized from RegisterMap::endpoint_count at construction. The actual writes in write_generic_config/write_functional_config land in regs_.generic_configs/regs_.functional_configs instead, which were never required to be the same length as endpoint_count. A RegisterMap with endpoint_count larger than those vectors let an in-range access check be followed by an out-of-bounds vector write (cpp-RCP-N2-01, #64). write_whole_map compounded this: it could install a replacement map with a different endpoint_count without resizing endpoint_owner_ to match, so a stale bounds check from the old map size could pass and then index out of bounds on the new, differently-sized vectors, or new endpoints could never get an owner assigned (cpp-RCP-N2-02, #65). Both are fixed via a shared size_invariant_holds() check (generic_configs .size() == functional_configs.size() == endpoint_count): the Ep0 constructor fails closed (sizes endpoint_owner_ to 0, rejecting all per-endpoint access) when a RegisterMap violates it, and write_whole_map now rejects an inconsistent replacement map outright and resizes endpoint_owner_ to match a valid one. write_generic_config/ write_functional_config additionally bounds-check against the actual vector being indexed as a second, independent layer of defense. Verified against AddressSanitizer/UndefinedBehaviorSanitizer: standalone repros reproduced a real SEGV/heap-buffer-overflow on the pre-fix code for both issues, and are clean after the fix. Added regression tests covering both fail-closed construction and write_whole_map resize/reject behavior, and a new "ASan/UBSan (regmap)" CI job that builds and runs test_regmap under -fsanitize=address,undefined on every PR. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- .github/workflows/ci.yml | 39 ++++++++++++ include/rcp/regmap.hpp | 73 ++++++++++++++++++++-- tests/test_regmap.cpp | 132 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 239 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c52e622..8d5a8a5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -153,6 +153,45 @@ jobs: find include tests -name '*.hpp' -o -name '*.cpp' | \ xargs clang-tidy -p build --warnings-as-errors='*' || true + # ── ASan/UBSan (regmap) ─────────────────────────────────────────────────────── + # Scoped to test_regmap: rcp/regmap.hpp's Ep0 does raw std::vector + # indexing across per-endpoint access-control checks and the config + # vectors those checks gate (cpp-RCP-N2-01/N2-02, issues #64/#65), so this + # is the header most worth sanitizer coverage on a fast, gating job. This + # is additive to build-and-test above, not a replacement for it. + asan-ubsan-regmap: + name: ASan/UBSan (regmap) + runs-on: ubuntu-22.04 + needs: build-and-test + steps: + - uses: actions/checkout@v4 + + - name: Install tools + run: | + sudo apt-get update -qq + sudo apt-get install -y cmake ninja-build + + - name: Configure (ASan + UBSan) + env: + CC: clang-14 + CXX: clang++-14 + run: | + cmake -B build-asan \ + -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_CXX_STANDARD=17 \ + -DCMAKE_CXX_FLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -g" \ + -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" \ + -G Ninja + + - name: Build test_regmap + run: cmake --build build-asan --target test_regmap --parallel + + - name: Run test_regmap under ASan/UBSan + env: + ASAN_OPTIONS: halt_on_error=1 + UBSAN_OPTIONS: halt_on_error=1,print_stacktrace=1 + run: ./build-asan/tests/test_regmap + # ── LCOV coverage ───────────────────────────────────────────────────────────── coverage: name: Coverage (LCOV) diff --git a/include/rcp/regmap.hpp b/include/rcp/regmap.hpp index 3bf0940..7dae451 100644 --- a/include/rcp/regmap.hpp +++ b/include/rcp/regmap.hpp @@ -329,8 +329,33 @@ class Ep0 { // ServerLifecycle::generic_config_locked/functional_config_locked). enum class ConfigBlock { Generic, Functional }; + // endpoint_owner_ is the vector every access-control check (below) + // bounds a target endpoint against, but the writes those checks gate + // land in regs.generic_configs/regs.functional_configs instead — two + // vectors this class does not own and cannot force the caller to keep + // in sync with endpoint_count. If they are not the same length as + // endpoint_count when this object is constructed, size endpoint_owner_ + // to 0 rather than to the untrustworthy endpoint_count: every non-EP0 + // access check then fails closed with invalid_parameter instead of + // handing out an in-range verdict that a subsequent write turns into an + // out-of-bounds write on the shorter/empty config vector (cpp-RCP-N2-01, + // issue #64). write_generic_config/write_functional_config additionally + // bounds-check against the actual vector being indexed as a second, + // independent layer of defense (see below). Ep0(RegisterMap& regs, lifecycle::ServerLifecycle& lc) noexcept - : regs_(regs), lifecycle_(lc), endpoint_owner_(regs.endpoint_count) {} + : regs_(regs), lifecycle_(lc), + endpoint_owner_(size_invariant_holds(regs) ? regs.endpoint_count : 0) {} + + // is_endpoint_table_consistent reports whether the size invariant this + // class's access checks rely on currently holds (regs_.generic_configs + // .size() == regs_.functional_configs.size() == regs_.endpoint_count). + // It is false only when the RegisterMap this Ep0 was constructed with + // violated the invariant and per-endpoint access has therefore been + // fail-closed (endpoint_owner_ sized to 0) until a valid replacement + // map is installed via write_whole_map. + bool is_endpoint_table_consistent() const noexcept { + return size_invariant_holds(regs_) && endpoint_owner_.size() == regs_.endpoint_count; + } // claim_root_client implements the exclusive root-client rule: once a // stream holds the root-client slot, a different stream's claim is @@ -402,23 +427,47 @@ class Ep0 { // call this. const RegisterMap& read_whole_map() const noexcept { return regs_; } - // write_whole_map is the root-client-only half of the same rule. + // write_whole_map is the root-client-only half of the same rule. Beyond + // the root-client check, it must also keep the size invariant every + // access check in this class depends on intact (cpp-RCP-N2-02, issue + // #65): a root client could otherwise install a replacement map whose + // generic_configs/functional_configs disagree with its own + // endpoint_count, or that simply differs in endpoint_count from the map + // endpoint_owner_ was last sized against, either of which lets a + // subsequent per-endpoint write pass a stale/incorrect bounds check and + // land out of bounds on the new vectors. So: reject an internally + // inconsistent replacement map outright, and resize endpoint_owner_ to + // match the (now validated) new endpoint_count as part of the same + // operation, discarding prior per-endpoint owner assignments — they + // describe endpoints in the map being replaced, not necessarily the + // same endpoints in the new one. std::error_code write_whole_map(size_t client, RegisterMap new_map) noexcept { if (!is_root_client(client)) return make_error_code(RegMapErrc::unauthorized_access); + if (!size_invariant_holds(new_map)) + return make_error_code(RegMapErrc::invalid_parameter); regs_ = std::move(new_map); + endpoint_owner_.assign(regs_.endpoint_count, std::nullopt); return {}; } // write_generic_config / write_functional_config apply the per-endpoint // write-access and per-block lock checks above before mutating the - // targeted endpoint's config block in the underlying RegisterMap. + // targeted endpoint's config block in the underlying RegisterMap. The + // explicit size check against the vector actually being indexed (rather + // than trusting check_write_access's endpoint_owner_-based verdict + // alone) is a second, independent layer of defense against cpp-RCP-N2-01 + // (issue #64): even if endpoint_owner_ and the config vectors were ever + // to drift apart again, these writes still cannot go out of bounds. std::error_code write_generic_config(size_t client, EndpointId target, EndpointGenericConfig cfg) noexcept { if (target == kEp0) return make_error_code(RegMapErrc::invalid_parameter); auto ec = check_write_access(client, target, ConfigBlock::Generic); if (ec) return ec; - regs_.generic_configs[static_cast(target - 1)] = std::move(cfg); + const auto idx = static_cast(target - 1); + if (idx >= regs_.generic_configs.size()) + return make_error_code(RegMapErrc::invalid_parameter); + regs_.generic_configs[idx] = std::move(cfg); return {}; } @@ -427,11 +476,25 @@ class Ep0 { if (target == kEp0) return make_error_code(RegMapErrc::invalid_parameter); auto ec = check_write_access(client, target, ConfigBlock::Functional); if (ec) return ec; - regs_.functional_configs[static_cast(target - 1)] = std::move(cfg); + const auto idx = static_cast(target - 1); + if (idx >= regs_.functional_configs.size()) + return make_error_code(RegMapErrc::invalid_parameter); + regs_.functional_configs[idx] = std::move(cfg); return {}; } private: + // size_invariant_holds is the shared root-invariant check both the + // constructor and write_whole_map enforce: every access-control check + // above bounds a target endpoint against endpoint_owner_ (sized from + // endpoint_count), but the writes those checks gate land in + // regs.generic_configs/regs.functional_configs — this must hold for + // that to be safe (cpp-RCP-N2-01/N2-02, issues #64/#65). + static bool size_invariant_holds(const RegisterMap& regs) noexcept { + return regs.generic_configs.size() == regs.endpoint_count && + regs.functional_configs.size() == regs.endpoint_count; + } + RegisterMap& regs_; lifecycle::ServerLifecycle& lifecycle_; std::optional root_client_; diff --git a/tests/test_regmap.cpp b/tests/test_regmap.cpp index 9ddc776..6e2f25a 100644 --- a/tests/test_regmap.cpp +++ b/tests/test_regmap.cpp @@ -349,6 +349,138 @@ TEST_CASE("Assigning an owner to EP0 itself is rejected as INVALID_PARAMETER", " REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter)); } +// ── Size-invariant enforcement (cpp-RCP-N2-01 / cpp-RCP-N2-02, issues #64/#65) ── +// Ep0's access checks (check_read_access/check_write_access/set_endpoint_owner) +// all bound a target endpoint against endpoint_owner_, which is sized from +// endpoint_count. The actual writes land in regs_.generic_configs / +// regs_.functional_configs instead. These tests confirm a RegisterMap whose +// endpoint_count disagrees with those vectors' lengths is rejected/fails +// closed rather than letting an in-range access check be followed by an +// out-of-bounds write. + +TEST_CASE("A RegisterMap whose config vectors disagree with endpoint_count fails closed instead of allowing OOB access", + "[regmap][REQ-REGMAP-013]") { + RegisterMap m; + m.endpoint_count = 4; + // generic_configs / functional_configs deliberately left empty/default + // -- exactly the mismatch issue #64 describes. + ServerLifecycle lc; + Ep0 ep0(m, lc); + + REQUIRE_FALSE(ep0.is_endpoint_table_consistent()); + + REQUIRE_FALSE(ep0.claim_root_client(1)); + + // Every non-EP0 access check must fail closed (INVALID_PARAMETER), not + // report a target in [1, endpoint_count] as valid. + auto ec_owner = ep0.set_endpoint_owner(1, 1); + REQUIRE(ec_owner); + REQUIRE(ec_owner == make_error_code(RegMapErrc::invalid_parameter)); + + auto ec_read = ep0.check_read_access(1); + REQUIRE(ec_read); + REQUIRE(ec_read == make_error_code(RegMapErrc::invalid_parameter)); + + EndpointGenericConfig gcfg; + auto ec_gwrite = ep0.write_generic_config(1, /*target=*/1, gcfg); + REQUIRE(ec_gwrite); + REQUIRE(ec_gwrite == make_error_code(RegMapErrc::invalid_parameter)); + + EndpointFunctionalConfig fcfg; + auto ec_fwrite = ep0.write_functional_config(1, /*target=*/1, fcfg); + REQUIRE(ec_fwrite); + REQUIRE(ec_fwrite == make_error_code(RegMapErrc::invalid_parameter)); + + // EP0 (whole-map) read/write are unaffected -- the invariant only + // gates per-endpoint access. + REQUIRE_FALSE(ep0.check_read_access(kEp0)); +} + +TEST_CASE("A RegisterMap with a smaller mismatch (nonzero but undersized config vectors) also fails closed", + "[regmap][REQ-REGMAP-013]") { + RegisterMap m; + m.endpoint_count = 4; + m.generic_configs.resize(1); + m.functional_configs.resize(1); + ServerLifecycle lc; + Ep0 ep0(m, lc); + + REQUIRE_FALSE(ep0.is_endpoint_table_consistent()); + REQUIRE_FALSE(ep0.claim_root_client(1)); + + EndpointGenericConfig cfg; + // target=4 would be in-range against endpoint_count (and against the + // pre-fix endpoint_owner_.size()==4), but generic_configs only has 1 + // slot -- this must not be allowed through. + auto ec = ep0.write_generic_config(1, /*target=*/4, cfg); + REQUIRE(ec); + REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter)); +} + +TEST_CASE("write_whole_map rejects a replacement map whose config vectors disagree with its own endpoint_count", + "[regmap][REQ-REGMAP-013]") { + auto map = make_map(2); + ServerLifecycle lc; + Ep0 ep0(map, lc); + REQUIRE_FALSE(ep0.claim_root_client(1)); + + RegisterMap bad; + bad.endpoint_count = 3; + bad.generic_configs.resize(1); // mismatched + bad.functional_configs.resize(3); + + auto ec = ep0.write_whole_map(1, bad); + REQUIRE(ec); + REQUIRE(ec == make_error_code(RegMapErrc::invalid_parameter)); + + // Rejected write must leave the live map untouched. + REQUIRE(ep0.read_whole_map().endpoint_count == 2); + REQUIRE(ep0.is_endpoint_table_consistent()); +} + +TEST_CASE("write_whole_map with a valid replacement map resizes endpoint_owner_ to match the new endpoint_count", + "[regmap][REQ-REGMAP-005]") { + auto map = make_map(2); + ServerLifecycle lc; + Ep0 ep0(map, lc); + REQUIRE_FALSE(ep0.set_endpoint_owner(1, 10)); + REQUIRE_FALSE(ep0.set_endpoint_owner(2, 20)); + REQUIRE_FALSE(ep0.claim_root_client(1)); + + // Grow: install a 4-endpoint replacement. Before the fix, endpoint_owner_ + // stayed sized at 2, so endpoint 4 could never get an owner (silent + // DoS) even though write_generic_config's own bounds check would have + // been the thing standing between a client and an OOB write. + auto bigger = make_map(4); + REQUIRE_FALSE(ep0.write_whole_map(1, bigger)); + REQUIRE(ep0.is_endpoint_table_consistent()); + + // Prior per-endpoint owner assignments are discarded by the whole-map + // replacement -- they described endpoints in the old map, not + // necessarily the same endpoints in the new one. + EndpointGenericConfig cfg; + auto ec_stale_owner = ep0.write_generic_config(/*client=*/10, /*target=*/1, cfg); + REQUIRE(ec_stale_owner); + REQUIRE(ec_stale_owner == make_error_code(RegMapErrc::unauthorized_access)); + + // The root client can write any endpoint in the new, larger range, + // including endpoint 4, which did not exist under the old map. + REQUIRE_FALSE(ep0.write_generic_config(1, /*target=*/4, cfg)); + REQUIRE(ep0.read_whole_map().generic_configs[3].request_queue_size == cfg.request_queue_size); + + // Shrink: install a 1-endpoint replacement. endpoint 4 must no longer + // be accepted as a write target -- this is exactly the stale-bounds + // scenario issue #65 describes (endpoint_owner_ too large for the new, + // shorter config vectors) if endpoint_owner_ were not resized down too. + auto smaller = make_map(1); + REQUIRE_FALSE(ep0.write_whole_map(1, smaller)); + REQUIRE(ep0.is_endpoint_table_consistent()); + + auto ec_oob = ep0.write_generic_config(1, /*target=*/4, cfg); + REQUIRE(ec_oob); + REQUIRE(ec_oob == make_error_code(RegMapErrc::invalid_parameter)); +} + // ── The four mandatory error codes are distinct ───────────────────────────────── TEST_CASE("The four mandatory register-map error codes are distinct values in their own category",