From f0e77c5b6e13dfd45ce0ead3610da4ed239864a0 Mon Sep 17 00:00:00 2001 From: Matt <47545907+SoundMatt@users.noreply.github.com> Date: Fri, 31 Jul 2026 21:44:18 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20v2.23.0=20=E2=80=94=20=C2=A713.1=20gener?= =?UTF-8?q?ic-config=20privilege=20escalation=20+=20fault-injection=20use-?= =?UTF-8?q?after-free?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unrelated critical defects, both found by an independent audit of this repository, fixed together because neither is large. 1. Privilege escalation on the generic config block (cpp-RCP-D2) §13.1 grants an RC Client other than the ROOT_CLIENT write access only to the functional config (EP_FUNC_config) of the endpoints allocated to it; §13.2 frames the counterpart block as the generic part of the endpoint register map, owned by the RC Server. Ep0::check_write_access collapsed that distinction: it applied one identical owner-based authorization test to both ConfigBlock::Generic and ConfigBlock::Functional, with only the *lock* branch — not the *authorization* branch — differing by block. write_generic_config therefore succeeded for any non-root client that merely owned the target endpoint. That is an escalation rather than a cosmetic gap because of what the generic block holds: hw_pin_indices (which physical pins the endpoint claims), request_queue_size/response_queue_size, and the per-endpoint ep_req_crc_enable/ep_ack_crc_enable/ep_response_crc_enable E2E-CRC toggles added at v2.6.0. A non-root client could repoint an owned endpoint's hardware pin mapping or silently switch off a safety mechanism — data the specification reserves to the RC Server and the root client. Authorization now depends on the targeted block: generic-block writes require the root client, functional-block writes accept root-or-owner. `block` keeps defaulting to ConfigBlock::Generic, which is now the stricter of the two, so a call site that forgets to name a block fails closed. write_functional_config, write_whole_map, check_read_access and the cpp-RCP-N2-01/N2-02 size invariants are untouched. tests/test_regmap.cpp's "A non-root client may only write the endpoint config it owns" was asserting the bug as correct behavior — it wrote the *generic* block as a non-root owner and required success. It is split in two: the owner grant is retested against the functional block where it belongs, and a new case pins the generic block as root-only, exercising write_generic_config and check_write_access directly (including the default-argument path) and asserting the refused write left the config untouched. The generic-block lock test now claims the root client first so the lock, not the new authorization check, is still what refuses that write. 2. Use-after-free in faultinject::Interceptor::pick_rule (cpp-RCP-D6) pick_rule returned a Rule* into its own std::vector and erased that very element before returning whenever the firing exhausted the rule's count. send() then dereferenced the erased element to read type and latency. Erasing a trailing element does not reallocate on common libstdc++/libc++ builds, so unsanitized builds read stale-but-mapped memory and appeared to work — which is why the existing count-expiry test never failed. The same pointer was also a data race: rules_ is guarded only for the duration of pick_rule(), so send() read into the container with no lock held while another thread could be reallocating it via add_rule, emptying it via clear_rules, or expiring rules of its own — in a class that holds a std::mutex precisely because it is meant to be used concurrently. pick_rule now returns std::optional, copied out while the lock is still held and before any erase; send() acts on that value. Rule is four scalars, so the copy is free. Verification Full local build and ctest (53/53 suites) pass. Both fixes were mutation- checked. Reverting the regmap authorization to the pre-fix owner-only test fails the new generic-block case. Building tests/test_faultinject with -fsanitize=address,undefined against the pre-fix header aborts with "AddressSanitizer: container-overflow" at the switch (rule->type) in send() — reached from the pre-existing "Slow rule adds latency" case, from the new single-shot expiry case, and from a worker thread in the new concurrent case — and passes clean against the fixed header. That the existing suite already reproduced the abort yet CI stayed green is itself a finding: the ASan/UBSan job was scoped to test_regmap alone. It now builds and runs test_faultinject under the same flags, so this class of defect is gated rather than fixed once. - regmap.hpp: block-dependent authorization in check_write_access; header and Ep0 class comments corrected (they described the collapsed rule). - faultinject.hpp: pick_rule returns std::optional; included. - test_regmap.cpp: owner-grant case rebased onto the functional block; new generic-block root-only case; generic-lock case claims root first. - test_faultinject.cpp: new same-call-expiry case (a Slow rule with count=1 must still apply its configured latency) and a new 8-thread concurrent case retiring 256 single-shot rules. - ci.yml: ASan/UBSan job extended to test_faultinject. - .fusa-reqs.json: REQ-REGMAP-005 restated with the Generic/Functional asymmetry (it encoded the buggy rule verbatim); REQ-REGMAP-006 no longer implies an owner may write the generic block; REQ-FI-005 states the copy-under-lock contract. - README.md/ROADMAP.md: milestone 67 added, GA renumbered to 68. No wire-format change and no public API signature change; pick_rule is private. The one observable behavior change is the intended one: a non-root client's generic-config write that previously succeeded now returns UNAUTHORIZED_ACCESS. kVersion/CMakeLists.txt bump 2.22.0 -> 2.23.0. Signed-off-by: Matt <47545907+SoundMatt@users.noreply.github.com> --- .fusa-reqs.json | 6 +-- .github/workflows/ci.yml | 33 ++++++++++---- CMakeLists.txt | 2 +- README.md | 2 +- ROADMAP.md | 89 ++++++++++++++++++++++++++++++++++++- include/rcp/faultinject.hpp | 45 +++++++++++++++---- include/rcp/regmap.hpp | 39 ++++++++++++---- include/rcp/version.hpp | 2 +- tests/test_faultinject.cpp | 76 +++++++++++++++++++++++++++++++ tests/test_regmap.cpp | 68 +++++++++++++++++++++++++--- 10 files changed, 322 insertions(+), 40 deletions(-) diff --git a/.fusa-reqs.json b/.fusa-reqs.json index 4006734..0a1be78 100644 --- a/.fusa-reqs.json +++ b/.fusa-reqs.json @@ -763,7 +763,7 @@ { "id": "REQ-FI-005", "title": "Count-based rule expires after N firings", - "text": "A rule with count > 0 shall auto-expire after count applications and be removed from the active list.", + "text": "A rule with count > 0 shall auto-expire after count applications and be removed from the active list. The application that retires a rule shall still take full effect, and rule selection shall yield a value copy taken under the interceptor's own lock rather than a reference into the mutable rule list, so that no caller reads a rule element that has been erased or relocated.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" @@ -2099,7 +2099,7 @@ { "id": "REQ-REGMAP-005", "title": "Per-endpoint write restriction for non-root clients", - "text": "Ep0::check_write_access (and the write_generic_config/write_functional_config calls built on it) shall permit a non-root client to write only the endpoint config it owns per Ep0::set_endpoint_owner, shall return RegMapErrc::unauthorized_access for any other endpoint, shall always permit the root client, and shall return RegMapErrc::invalid_parameter for an out-of-range endpoint id.", + "text": "Ep0::check_write_access (and the write_generic_config/write_functional_config calls built on it) shall permit a non-root client to write only the FUNCTIONAL config block of an endpoint it owns per Ep0::set_endpoint_owner (TC18 section 13.1 grants a non-ROOT_CLIENT write access to EP_FUNC_config only; section 13.2 places the generic block under the RC Server's ownership), shall return RegMapErrc::unauthorized_access for a non-root client writing the GENERIC config block of any endpoint including one it owns, shall return RegMapErrc::unauthorized_access for a non-root client writing either block of an endpoint it does not own, shall always permit the root client, and shall return RegMapErrc::invalid_parameter for an out-of-range endpoint id.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" @@ -2107,7 +2107,7 @@ { "id": "REQ-REGMAP-006", "title": "Config-block locks are enforced independently by EP0", - "text": "Ep0::write_generic_config shall return RegMapErrc::locked_mem_access once ServerLifecycle::generic_config_locked is true, and Ep0::write_functional_config shall return RegMapErrc::locked_mem_access once ServerLifecycle::functional_config_locked is true, even when called by an otherwise-authorized owning or root client.", + "text": "Ep0::write_generic_config shall return RegMapErrc::locked_mem_access once ServerLifecycle::generic_config_locked is true, and Ep0::write_functional_config shall return RegMapErrc::locked_mem_access once ServerLifecycle::functional_config_locked is true, even when called by a client that REQ-REGMAP-005 would otherwise authorize for that block.", "standard": "iso26262", "level": "ASIL-B", "asil": "ASIL-B" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aac07a0..1a18f63 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -209,14 +209,23 @@ jobs: find tests -name '*.cpp' | \ xargs clang-tidy -p build --warnings-as-errors='*' - # ── 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 + faultinject) ───────────────────────────────────────── + # Scoped to the two suites whose headers hand out raw references into + # containers they also mutate: + # + # - 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). + # - rcp/faultinject.hpp's Interceptor picks a rule out of a std::vector it + # erases from in the same pass (cpp-RCP-D6). That defect was live and + # the *existing* test_faultinject cases already reproduced it under + # ASan — this job simply wasn't running them, which is why CI stayed + # green. Keeping it in scope makes that class of defect gated rather + # than fixed once. + # + # This is additive to build-and-test above, not a replacement for it. asan-ubsan-regmap: - name: ASan/UBSan (regmap) + name: ASan/UBSan (regmap + faultinject) runs-on: ubuntu-22.04 needs: build-and-test steps: @@ -239,8 +248,8 @@ jobs: -DCMAKE_EXE_LINKER_FLAGS="-fsanitize=address,undefined" \ -G Ninja - - name: Build test_regmap - run: cmake --build build-asan --target test_regmap --parallel + - name: Build test_regmap and test_faultinject + run: cmake --build build-asan --target test_regmap test_faultinject --parallel - name: Run test_regmap under ASan/UBSan env: @@ -248,6 +257,12 @@ jobs: UBSAN_OPTIONS: halt_on_error=1,print_stacktrace=1 run: ./build-asan/tests/test_regmap + - name: Run test_faultinject under ASan/UBSan + env: + ASAN_OPTIONS: halt_on_error=1 + UBSAN_OPTIONS: halt_on_error=1,print_stacktrace=1 + run: ./build-asan/tests/test_faultinject + # ── LCOV coverage ───────────────────────────────────────────────────────────── coverage: name: Coverage (LCOV) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5ef3b10..12a8c7f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,7 +1,7 @@ cmake_minimum_required(VERSION 3.21) project(cpp-rcp - VERSION 2.22.0 + VERSION 2.23.0 DESCRIPTION "Remote Control Protocol for automotive zonal architecture (C++)" HOMEPAGE_URL "https://github.com/SoundMatt/cpp-RCP" LANGUAGES CXX diff --git a/README.md b/README.md index 91af81a..8b3ad83 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ reference. |---|---| | `` | TC18 wire codec, framing half — IEEE 1722 AVTPDU (NTSCF/TSCF) header framing, `StreamId`, `ByteBusId` | | `` | TC18 wire codec, message half — ACF_ABB/ACF_GBB message format, `AcfMessageInfo`. ACF_ABB Message Info is 8 contiguous octets; ACF_GBB is 16, with the 64-bit `message_timestamp` spliced between the header's two quadlets (octets 0–3 quadlet 0, 4–11 timestamp, 12–15 quadlet 1) — corrected in v2.22.0, see ROADMAP.md milestone 66 | -| `` | RC Server register-map model (generic + functional config, EP0) | +| `` | RC Server register-map model (generic + functional config, EP0). The two config blocks differ in who may write them: the generic block is the RC Server's own and is root-client-only, while a non-root client may write the functional block of an endpoint allocated to it (§13.1/§13.2) — corrected in v2.23.0, see ROADMAP.md milestone 67 | | `` | RC Server lifecycle state machine | | `` | Conditional-request taxonomy (compound, triggered, timed, chained, ...) and sequencers | | `` | Shared primitives used codebase-wide: the `Errc` sentinel error category, `Context` (a `relay::Context` alias), and `Loan` (a generic RAII buffer-loan holder) | diff --git a/ROADMAP.md b/ROADMAP.md index 9f07e6e..a295f1c 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -136,6 +136,7 @@ work, since every endpoint type's functional config block depends on it. | **Phase 16** | v2.20.0 | TC18 conformance fix pass II | `acf_msg_length` actually computed; multiple-requests-in-one-frame (§12.9.1.1) supported; error responses carry a Table 27 error code; GPIO exact-length payload check; ISELED gains a real wire codec; PWM_IN rising/falling-edge triggers; `control_data_length` validated | | **Phase 16** | v2.21.0 | L2 (native Ethernet) transport + UDP Annex J conformance fix | New `l2.hpp` raw-Ethernet transport (EtherType 0x22F0); `udp.hpp` gains Annex J's 4-byte encapsulation sequence number and port 17221 default | | **Phase 16** | v2.22.0 | ACF_GBB `message_timestamp` wire-position fix | The 64-bit `message_timestamp` is spliced *between* the ACF Message Info header's two quadlets (octet 4), not appended after both (octet 8); `e2e.hpp` CRC coverage follows; wire-breaking for ACF_GBB only | +| **Phase 16** | v2.23.0 | §13.1 config-block privilege separation + fault-injection use-after-free | `regmap::Ep0` generic config is root-client-only (owning an endpoint grants the *functional* block only); `faultinject::pick_rule` no longer hands `send()` a pointer to an erased vector element; ASan/UBSan CI job extended to `test_faultinject` | | **Phase 16** | v3.0.0 | **TC18 RCP — General Availability** | First release where cpp-RCP *is* the OPEN Alliance TC18 Remote Control Protocol | --- @@ -2096,7 +2097,93 @@ strictly a move toward the interoperability GA requires; `kVersion`/ -bump convention milestones 63/64/65 used for their own wire-format corrections during this pre-GA v2.x line. -### 67. TC18 RCP — General Availability (v3.0.0) +### 67. §13.1 Config-Block Privilege Separation + Fault-Injection Use-After-Free (v2.23.0) + +Two unrelated defects, both found by an independent audit of this +repository, both fixed together because both are critical and neither is +large: one lets a non-root RC Client write register-map data the +specification reserves to the ROOT_CLIENT, the other is a genuine +use-after-free. + +**The access-control bug (cpp-RCP-D2).** §13.1 grants an RC Client other +than the ROOT_CLIENT write access only to the *functional* config +(`EP_FUNC_config`) of the endpoints allocated to it. §13.2 frames the +counterpart block as the generic part of the endpoint register map, owned +by the RC Server. `regmap::Ep0::check_write_access` collapsed that +distinction: it applied one identical owner-based authorization test to +both `ConfigBlock::Generic` and `ConfigBlock::Functional`, with only the +*lock* branch — not the *authorization* branch — differing by block. So +`write_generic_config` succeeded for any non-root client that merely owned +the target endpoint. + +That is a privilege escalation, not a cosmetic conformance gap, because of +what the generic block holds: `hw_pin_indices` (which physical pins the +endpoint claims), `request_queue_size`/`response_queue_size`, and the +per-endpoint `ep_req_crc_enable`/`ep_ack_crc_enable`/`ep_rsp_crc_enable` +E2E-CRC toggles added at v2.6.0. A non-root client could therefore repoint +an endpoint's hardware pin mapping or silently switch off a safety +mechanism on an endpoint it happens to own — all data the specification +puts under the RC Server's and root client's exclusive control. + +The fix is the asymmetry itself: authorization now depends on the targeted +block, so generic-block writes require the root client and functional-block +writes accept root-or-owner. `block` keeps defaulting to +`ConfigBlock::Generic`, which is now the *stricter* of the two, so a call +site that forgets to name a block fails closed. `write_functional_config`, +`write_whole_map`, `check_read_access`, and the cpp-RCP-N2-01/N2-02 size +invariants are untouched. + +The pre-existing test `tests/test_regmap.cpp` case "A non-root client may +only write the endpoint config it owns" was asserting the bug as correct +behavior — it wrote the *generic* block as a non-root owner and required +success. It is split in two: the owner grant is retested against the +functional block where it belongs, and a new case pins the generic block as +root-only, checking both `write_generic_config` and `check_write_access` +directly (including the default-argument path) and asserting the refused +write left the config untouched. The generic-block lock test now claims the +root client first, so the lock is still what refuses that write rather than +the new authorization check masking it. + +**The use-after-free (cpp-RCP-D6).** `faultinject::Interceptor::pick_rule` +returned a `Rule*` pointing into its own `std::vector` and erased that +very element before returning whenever the firing exhausted the rule's +count. `send()` then dereferenced the erased element to read `type` and +`latency`. Erasing a trailing element does not reallocate on common +libstdc++/libc++ builds, so unsanitized builds read stale-but-mapped memory +and appeared to work — which is exactly why the existing count-expiry test +never failed. The same pointer was also a data race: `rules_` is guarded +only for the duration of `pick_rule()`, so `send()` read into the container +with no lock held while another thread could be reallocating it via +`add_rule`, emptying it via `clear_rules`, or expiring rules of its own — +in a class that holds a `std::mutex` precisely because it is meant to be +used concurrently. + +`pick_rule` now returns `std::optional`, copied out while the lock is +still held and before any erase; `send()` acts on that value. `Rule` is four +scalars, so the copy is free. + +**Verification.** Full local build and `ctest` (53/53 suites) pass. Both +fixes were mutation-checked. For cpp-RCP-D6 that check is the more +interesting one: building `tests/test_faultinject` with +`-fsanitize=address,undefined` against the *pre-fix* header aborts with +`AddressSanitizer: container-overflow` at the `switch (rule->type)` in +`send()` — reached from the pre-existing "Slow rule adds latency" case, from +the new single-shot expiry case, and from a worker thread in the new +concurrent case — and passes clean against the fixed header. That the +existing suite already reproduced the abort, yet CI stayed green, is itself +the finding: the ASan/UBSan job was scoped to `test_regmap` alone. It now +builds and runs `test_faultinject` under the same flags, so this class of +defect is gated rather than merely fixed once. + +**Compatibility.** No wire-format change and no public API signature change. +`pick_rule` is private, so its return type is an implementation detail. The +one observable behavior change is the intended one: a non-root client's +generic-config write that previously succeeded now returns +`UNAUTHORIZED_ACCESS`. `kVersion`/`CMakeLists.txt` bump 2.22.0 → 2.23.0, +the same minor-bump convention milestones 63/64/65/66 used during this +pre-GA v2.x line. + +### 68. TC18 RCP — General Availability (v3.0.0) - First release where cpp-RCP's `RCP` conforms to the OPEN Alliance TC18 Remote Control Protocol Specification at the wire level: an RC Client diff --git a/include/rcp/faultinject.hpp b/include/rcp/faultinject.hpp index d224b9e..c50cb9e 100644 --- a/include/rcp/faultinject.hpp +++ b/include/rcp/faultinject.hpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -82,7 +83,11 @@ class Interceptor { acf::AcfMessageInfo& out, std::vector& out_payload) { if (closed_.load(std::memory_order_acquire)) return ErrClosed; - Rule* rule = pick_rule(); + // pick_rule returns a *value* copy rather than a pointer into + // rules_: the element it describes may already have been erased by + // the time this function reads it, and rules_ is only mutex-guarded + // inside pick_rule() (cpp-RCP-D6 — see pick_rule below). + const std::optional rule = pick_rule(); if (!rule) return inner_(ctx, req, payload, out, out_payload); switch (rule->type) { @@ -118,22 +123,44 @@ class Interceptor { std::mutex mu_; std::vector rules_; - // Returns the first active rule (or nullptr) and decrements its count. - Rule* pick_rule() { + // Returns a copy of the first active rule (or std::nullopt) after + // accounting for this firing against its count. + // + // The copy is not incidental (cpp-RCP-D6). This used to return `Rule*` + // pointing at the live vector element, which was wrong twice over: + // + // 1. Use-after-free. A rule whose count is exhausted by *this* firing is + // erased here, and the pointer returned to send() then referred to an + // erased element — send() dereferences it immediately (rule->type, + // rule->latency). erase() of a trailing element does not reallocate + // on common libstdc++/libc++ builds, so an unsanitized build usually + // read stale-but-mapped memory and appeared to work; AddressSanitizer + // reports it as a container-overflow. + // 2. Data race. rules_ is only guarded for the duration of this + // function; the lock is released on return, so a pointer into rules_ + // was read by send() with no lock held while another thread could be + // concurrently add_rule()ing (reallocating the buffer), + // clear_rules()ing, or expiring rules of its own. This class holds a + // std::mutex precisely because it is meant to be used concurrently. + // + // Copying the Rule out while the lock is still held closes both: the + // value send() acts on is its own, and it stays valid however rules_ is + // subsequently mutated. Rule is four scalars, so the copy is free. + std::optional pick_rule() { std::lock_guard lk(mu_); for (auto it = rules_.begin(); it != rules_.end(); ) { if (it->count == 0) { it = rules_.erase(it); continue; } - Rule* r = &(*it); - ++r->fired; - if (r->count > 0 && r->fired >= r->count) { - it = rules_.erase(it); + ++it->fired; + const Rule picked = *it; // copy out before any possible erase + if (picked.count > 0 && picked.fired >= picked.count) { + rules_.erase(it); } - return r; + return picked; } - return nullptr; + return std::nullopt; } }; diff --git a/include/rcp/regmap.hpp b/include/rcp/regmap.hpp index 7dae451..7403add 100644 --- a/include/rcp/regmap.hpp +++ b/include/rcp/regmap.hpp @@ -315,8 +315,10 @@ struct RegisterMap { // endpoint (as EP0) for whole-register-map reads and writes. Every client may // read the whole map. Only one client — the root client, identified by // svr_root_client_index — may write the whole map through EP0; every other -// client is restricted to writing the generic/functional config of the -// endpoint(s) it owns. +// client is restricted to writing the *functional* config of the endpoint(s) +// it owns. The generic config block is the RC Server's own and is writable by +// the root client alone, no matter who owns the endpoint (TC18 §13.1/§13.2, +// cpp-RCP-D2 — see check_write_access below). // // "Client" here is identified by an opaque index the embedding transport // assigns per connected stream; this header does not know or care how that @@ -398,10 +400,25 @@ class Ep0 { } // check_write_access enforces: EP0 (whole-map) writes require the root - // client; a non-EP0 endpoint's write requires either the root client or - // that endpoint's assigned owner; and a write to whichever config block - // is targeted is rejected while the current lifecycle state has that - // specific block locked, regardless of who is asking. + // client; a non-EP0 endpoint's write requires the root client, or — for + // the *functional* config block only — that endpoint's assigned owner; + // and a write to whichever config block is targeted is rejected while + // the current lifecycle state has that specific block locked, regardless + // of who is asking. + // + // The Generic/Functional asymmetry is the access-control rule TC18 §13.1 + // states (extraction: for RC Clients other than the ROOT_CLIENT, write + // access is granted only to the *functional* config of endpoints + // allocated to that client) and §13.2's own framing reinforces (the + // generic part of the endpoint register map is owned by the RC Server, + // not by the allocated client). Owning an endpoint therefore does NOT + // confer write access to that endpoint's generic block: HW pin mapping, + // request/response queue sizing, and the E2E-CRC enable toggles live + // there, and a non-root client that could rewrite them would be able to + // repoint another endpoint's hardware pins or silently disable a safety + // mechanism (cpp-RCP-D2). Note that `block` defaults to + // ConfigBlock::Generic, i.e. to the stricter of the two checks, so a + // caller that forgets to name a block fails closed rather than open. std::error_code check_write_access(size_t client, EndpointId target, ConfigBlock block = ConfigBlock::Generic) const noexcept { if (target == kEp0) { @@ -411,7 +428,10 @@ class Ep0 { if (target > endpoint_owner_.size()) return make_error_code(RegMapErrc::invalid_parameter); const auto& owner = endpoint_owner_[static_cast(target - 1)]; - if (!is_root_client(client) && (!owner.has_value() || *owner != client)) + const bool owns = owner.has_value() && *owner == client; + const bool authorized = + is_root_client(client) || (block == ConfigBlock::Functional && owns); + if (!authorized) return make_error_code(RegMapErrc::unauthorized_access); const bool locked = (block == ConfigBlock::Generic) ? lifecycle_.generic_config_locked() @@ -453,7 +473,10 @@ class Ep0 { // 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. The + // targeted endpoint's config block in the underlying RegisterMap. The two + // are deliberately not symmetric in who may call them: the generic block + // is root-client-only, the functional block is root-or-owner (TC18 + // §13.1/§13.2 — see check_write_access above, cpp-RCP-D2). 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 diff --git a/include/rcp/version.hpp b/include/rcp/version.hpp index 5499f74..cb0fb8d 100644 --- a/include/rcp/version.hpp +++ b/include/rcp/version.hpp @@ -6,6 +6,6 @@ namespace rcp { // Semantic version of the cpp-RCP implementation (matches the latest git tag). -constexpr std::string_view kVersion = "2.22.0"; +constexpr std::string_view kVersion = "2.23.0"; } // namespace rcp diff --git a/tests/test_faultinject.cpp b/tests/test_faultinject.cpp index 890ae58..9ec3d94 100644 --- a/tests/test_faultinject.cpp +++ b/tests/test_faultinject.cpp @@ -18,8 +18,11 @@ #include "rcp/faultinject.hpp" #include "rcp/mock.hpp" +#include +#include #include #include +#include using namespace rcp; using namespace std::chrono_literals; @@ -133,6 +136,79 @@ TEST_CASE("FaultInject rule expires after count sends", "[faultinject][REQ-FI-00 REQUIRE_FALSE(fi.send(Context::background(), req, {}, out, out_payload)); // passes } +// ── Expiry on the firing call must not read an erased rule (cpp-RCP-D6) ─────── +// pick_rule() used to return a Rule* into its own std::vector and erase that +// very element before returning, so send() dereferenced an erased element to +// read type/latency. An unsanitized build usually got away with it (erasing a +// trailing element does not reallocate), which is why the count-expiry test +// above never caught it; AddressSanitizer reports a container-overflow. These +// cases pin the fixed contract: the rule is applied from a copy taken while +// the lock is held, so its effect is correct on the exact call that retires +// it, and remains correct with concurrent callers. + +TEST_CASE("A rule that expires on the same call it fires is still applied correctly", + "[faultinject][REQ-FI-005]") { + faultinject::Interceptor fi(make_mock_fn(make_server())); + + faultinject::Rule r; + r.type = faultinject::FaultType::Slow; + r.latency = 60ms; + r.count = 1; // fires once and is retired during that same pick_rule() + fi.add_rule(r); + + auto req = gpio_read_request(); + acf::AcfMessageInfo out; + std::vector out_payload; + + // The Slow rule's latency is read after the rule has been erased; it must + // still be the 60ms that was configured, not whatever the freed slot held. + auto start = std::chrono::steady_clock::now(); + REQUIRE_FALSE(fi.send(Context::background(), req, {}, out, out_payload)); + auto elapsed = std::chrono::steady_clock::now() - start; + REQUIRE(elapsed >= 50ms); + + // The rule really is gone: the next call passes straight through. + start = std::chrono::steady_clock::now(); + REQUIRE_FALSE(fi.send(Context::background(), req, {}, out, out_payload)); + REQUIRE(std::chrono::steady_clock::now() - start < 50ms); +} + +TEST_CASE("Concurrent senders retiring count-limited rules do not read erased rules", + "[faultinject][REQ-FI-005]") { + faultinject::Interceptor fi(make_mock_fn(make_server())); + + // Many single-shot rules, so nearly every send() retires the element it + // just picked while other threads are picking/erasing/appending too. + constexpr int kRules = 256; + constexpr int kThreads = 8; + for (int i = 0; i < kRules; ++i) { + faultinject::Rule r; + r.type = faultinject::FaultType::Timeout; + r.count = 1; + fi.add_rule(r); + } + + std::atomic timeouts{0}; + std::vector threads; + threads.reserve(kThreads); + for (int t = 0; t < kThreads; ++t) { + threads.emplace_back([&] { + for (int i = 0; i < kRules / kThreads; ++i) { + auto req = gpio_read_request(); + acf::AcfMessageInfo out; + std::vector out_payload; + if (fi.send(Context::background(), req, {}, out, out_payload) == ErrTimeout) + timeouts.fetch_add(1, std::memory_order_relaxed); + } + }); + } + for (auto& th : threads) th.join(); + + // Every send consumed exactly one single-shot Timeout rule; a torn read of + // an erased rule would surface here as a wrong (or garbage) FaultType. + REQUIRE(timeouts.load() == kRules); +} + // ── clear_rules ─────────────────────────────────────────────────────────────── TEST_CASE("FaultInject::clear_rules removes all active rules", "[faultinject][REQ-FI-006]") { diff --git a/tests/test_regmap.cpp b/tests/test_regmap.cpp index 6e2f25a..3e4dc59 100644 --- a/tests/test_regmap.cpp +++ b/tests/test_regmap.cpp @@ -119,25 +119,75 @@ TEST_CASE("A second, distinct client cannot claim the root-client slot while it // ── Per-endpoint write restriction ────────────────────────────────────────────── -TEST_CASE("A non-root client may only write the endpoint config it owns", "[regmap][REQ-REGMAP-005]") { +TEST_CASE("A non-root client may only write the functional config of the endpoint it owns", + "[regmap][REQ-REGMAP-005]") { auto map = make_map(2); ServerLifecycle lc; Ep0 ep0(map, lc); REQUIRE_FALSE(ep0.set_endpoint_owner(1, /*client=*/10)); REQUIRE_FALSE(ep0.set_endpoint_owner(2, /*client=*/20)); - EndpointGenericConfig cfg; - cfg.request_queue_size = 8; + EndpointFunctionalConfig cfg; + cfg.data = {0xAA}; - REQUIRE_FALSE(ep0.write_generic_config(/*client=*/10, /*target=*/1, cfg)); - REQUIRE(ep0.read_whole_map().generic_configs[0].request_queue_size == 8); + REQUIRE_FALSE(ep0.write_functional_config(/*client=*/10, /*target=*/1, cfg)); + REQUIRE(ep0.read_whole_map().functional_configs[0].data == std::vector{0xAA}); - auto ec = ep0.write_generic_config(/*client=*/10, /*target=*/2, cfg); + // An endpoint owned by somebody else is off limits. + auto ec = ep0.write_functional_config(/*client=*/10, /*target=*/2, cfg); REQUIRE(ec); REQUIRE(ec == make_error_code(RegMapErrc::unauthorized_access)); - // The root client, once claimed, may write any endpoint's config. + // The root client, once claimed, may write any endpoint's functional config. + REQUIRE_FALSE(ep0.claim_root_client(/*client=*/99)); + REQUIRE_FALSE(ep0.write_functional_config(/*client=*/99, /*target=*/2, cfg)); + REQUIRE(ep0.read_whole_map().functional_configs[1].data == std::vector{0xAA}); +} + +// The generic config block is *not* covered by the owner grant above: TC18 +// §13.1 grants a non-ROOT_CLIENT write access only to the functional config +// of the endpoints allocated to it, and §13.2 describes the generic part of +// the endpoint register map as owned by the RC Server. Before cpp-RCP-D2 this +// implementation applied one identical owner-based check to both blocks, so +// any client that merely owned an endpoint could rewrite that endpoint's HW +// pin mapping, queue sizing, and E2E-CRC enable toggles — a privilege +// escalation into data reserved to the root client. +TEST_CASE("Owning an endpoint does not grant a non-root client write access to its generic config", + "[regmap][REQ-REGMAP-005]") { + auto map = make_map(2); + ServerLifecycle lc; + Ep0 ep0(map, lc); + REQUIRE_FALSE(ep0.set_endpoint_owner(1, /*client=*/10)); + REQUIRE_FALSE(ep0.set_endpoint_owner(2, /*client=*/20)); + + EndpointGenericConfig cfg; + cfg.request_queue_size = 8; + cfg.hw_pin_indices = {7}; + + // The owner of endpoint 1 is refused on its *own* endpoint's generic block. + auto ec_own = ep0.write_generic_config(/*client=*/10, /*target=*/1, cfg); + REQUIRE(ec_own); + REQUIRE(ec_own == make_error_code(RegMapErrc::unauthorized_access)); + REQUIRE(ep0.read_whole_map().generic_configs[0].request_queue_size != 8); + REQUIRE(ep0.read_whole_map().generic_configs[0].hw_pin_indices.empty()); + + // ...and, as before, on an endpoint owned by somebody else. + auto ec_other = ep0.write_generic_config(/*client=*/10, /*target=*/2, cfg); + REQUIRE(ec_other); + REQUIRE(ec_other == make_error_code(RegMapErrc::unauthorized_access)); + + // check_write_access reports the same asymmetry directly, and defaults to + // the stricter (generic) check when no block is named. + REQUIRE(ep0.check_write_access(/*client=*/10, /*target=*/1, Ep0::ConfigBlock::Generic) == + make_error_code(RegMapErrc::unauthorized_access)); + REQUIRE(ep0.check_write_access(/*client=*/10, /*target=*/1) == + make_error_code(RegMapErrc::unauthorized_access)); + REQUIRE_FALSE(ep0.check_write_access(/*client=*/10, /*target=*/1, Ep0::ConfigBlock::Functional)); + + // Only the root client may write the generic block. REQUIRE_FALSE(ep0.claim_root_client(/*client=*/99)); + REQUIRE_FALSE(ep0.write_generic_config(/*client=*/99, /*target=*/1, cfg)); + REQUIRE(ep0.read_whole_map().generic_configs[0].request_queue_size == 8); REQUIRE_FALSE(ep0.write_generic_config(/*client=*/99, /*target=*/2, cfg)); REQUIRE(ep0.read_whole_map().generic_configs[1].request_queue_size == 8); } @@ -161,6 +211,10 @@ TEST_CASE("Generic config writes are refused once the lifecycle locks the generi auto map = make_map(1); ServerLifecycle lc; Ep0 ep0(map, lc); + // The generic block is root-client-only (TC18 §13.1/§13.2, cpp-RCP-D2), + // so the lock is what has to refuse this write — use the root client, or + // the authorization check would mask the lock check being tested. + REQUIRE_FALSE(ep0.claim_root_client(10)); REQUIRE_FALSE(ep0.set_endpoint_owner(1, 10)); REQUIRE_FALSE(lc.advance(ServerState::HwConfigured));