From b9c1f7947d38ae6ca439ab808119cabdfcd89e24 Mon Sep 17 00:00:00 2001 From: Larry Ruckman Date: Fri, 29 May 2026 03:25:21 -0700 Subject: [PATCH] protocols/rssi: clamp peer-advertised connection parameters The RSSI SYN/SYN-ACK handshake adopted the peer's advertised parameters verbatim. An out-of-range peer value (zero outstanding segments, zero timeouts, a sub-header-size segment, or a window larger than the locally offered maximum) could drive the controller into illegal state -- for example curMaxBuffers == 0 makes the applicationRx outbound wait loop spin forever. Add Controller::clampPeerParams() and apply it in stateClosedWait() after the peer parameters are adopted, clamping to the locally legal range rather than rejecting the connection. This preserves backward/forward compatibility with conformant peers (including SLAC firmware, which never advertises invalid parameters) while hardening against a malformed SYN. Mirrors the firmware validPeerParams/clampWindowSize/clampBufferSize checks in surf protocols/rssi/v1/rtl/RssiConnFsm.vhd. Add a doctest covering the clamp helper (cpp-core, no-python), wired into the existing tests/cpp ctest suite. --- include/rogue/protocols/rssi/Controller.h | 34 ++++ src/rogue/protocols/rssi/Controller.cpp | 58 ++++++ tests/cpp/protocols/CMakeLists.txt | 1 + tests/cpp/protocols/rssi/CMakeLists.txt | 7 + tests/cpp/protocols/rssi/test_param_clamp.cpp | 178 ++++++++++++++++++ 5 files changed, 278 insertions(+) create mode 100644 tests/cpp/protocols/rssi/CMakeLists.txt create mode 100644 tests/cpp/protocols/rssi/test_param_clamp.cpp diff --git a/include/rogue/protocols/rssi/Controller.h b/include/rogue/protocols/rssi/Controller.h index a3c459d14b..2be5ebb7df 100644 --- a/include/rogue/protocols/rssi/Controller.h +++ b/include/rogue/protocols/rssi/Controller.h @@ -401,6 +401,40 @@ class Controller : public rogue::EnableSharedFromThis= 1. + * @param retranTout Peer retransmit timeout; clamped to >= 1. + * @param nullTout Peer null-segment timeout; clamped to >= 1. + * @param locMaxBuffers Locally offered max outstanding segments (upper bound). + * @param locMaxSegment Locally offered max segment size (upper bound). + * @return True if any parameter was modified, false if all were already legal. + */ + static bool clampPeerParams(uint8_t& maxBuffers, + uint16_t& maxSegment, + uint16_t& cumAckTout, + uint16_t& retranTout, + uint16_t& nullTout, + uint8_t locMaxBuffers, + uint16_t locMaxSegment); + /** @brief Resets runtime counters. */ void resetCounters(); diff --git a/src/rogue/protocols/rssi/Controller.cpp b/src/rogue/protocols/rssi/Controller.cpp index 435d1b181e..ebed526a3b 100644 --- a/src/rogue/protocols/rssi/Controller.cpp +++ b/src/rogue/protocols/rssi/Controller.cpp @@ -696,6 +696,51 @@ int8_t rpr::Controller::retransmit(uint8_t id) { return 1; } +//! Clamp peer-advertised connection parameters to the locally legal range +bool rpr::Controller::clampPeerParams(uint8_t& maxBuffers, + uint16_t& maxSegment, + uint16_t& cumAckTout, + uint16_t& retranTout, + uint16_t& nullTout, + uint8_t locMaxBuffers, + uint16_t locMaxSegment) { + bool clamped = false; + + // Max outstanding segments: at least 1, never more than locally offered + if (maxBuffers == 0) { + maxBuffers = 1; + clamped = true; + } else if (maxBuffers > locMaxBuffers) { + maxBuffers = locMaxBuffers; + clamped = true; + } + + // Max segment size: must hold at least a header, never more than locally offered + if (maxSegment < static_cast(rpr::Header::HeaderSize)) { + maxSegment = static_cast(rpr::Header::HeaderSize); + clamped = true; + } else if (maxSegment > locMaxSegment) { + maxSegment = locMaxSegment; + clamped = true; + } + + // Timeouts: a zero value would make the corresponding timer expire immediately + if (cumAckTout == 0) { + cumAckTout = 1; + clamped = true; + } + if (retranTout == 0) { + retranTout = 1; + clamped = true; + } + if (nullTout == 0) { + nullTout = 1; + clamped = true; + } + + return clamped; +} + //! Convert rssi time to microseconds void rpr::Controller::convTime(struct timeval& tme, uint32_t rssiTime) { float units = std::pow(10, -TimeoutUnit); @@ -790,6 +835,19 @@ struct timeval& rpr::Controller::stateClosedWait() { curMaxCumAck_ = head->maxCumulativeAck; lastAckRx_ = head->acknowledge; + // Clamp peer-advertised parameters to the locally legal range before + // they are converted to timers or used as window/buffer bounds. A + // conformant peer (including SLAC firmware) is unaffected. + if (clampPeerParams(curMaxBuffers_, + curMaxSegment_, + curCumAckTout_, + curRetranTout_, + curNullTout_, + locMaxBuffers_, + locMaxSegment_)) { + log_->warning("Clamped out-of-range peer RSSI parameters. server=%d", server_); + } + // Convert times convTime(retranToutD1_, curRetranTout_); convTime(cumAckToutD1_, curCumAckTout_); diff --git a/tests/cpp/protocols/CMakeLists.txt b/tests/cpp/protocols/CMakeLists.txt index c25310d973..5c5142a67c 100644 --- a/tests/cpp/protocols/CMakeLists.txt +++ b/tests/cpp/protocols/CMakeLists.txt @@ -1,4 +1,5 @@ add_subdirectory(packetizer) +add_subdirectory(rssi) if (NOT NO_PYTHON) add_subdirectory(xilinx) diff --git a/tests/cpp/protocols/rssi/CMakeLists.txt b/tests/cpp/protocols/rssi/CMakeLists.txt new file mode 100644 index 0000000000..aced63ab22 --- /dev/null +++ b/tests/cpp/protocols/rssi/CMakeLists.txt @@ -0,0 +1,7 @@ +rogue_add_cpp_test(rogue-cpp-protocols-rssi-param-clamp + SOURCES + test_param_clamp.cpp + LABELS + cpp-core + no-python +) diff --git a/tests/cpp/protocols/rssi/test_param_clamp.cpp b/tests/cpp/protocols/rssi/test_param_clamp.cpp new file mode 100644 index 0000000000..71c854342f --- /dev/null +++ b/tests/cpp/protocols/rssi/test_param_clamp.cpp @@ -0,0 +1,178 @@ +/** + * ---------------------------------------------------------------------------- + * Company : SLAC National Accelerator Laboratory + * ---------------------------------------------------------------------------- + * Description: + * Regression: rpr::Controller::clampPeerParams() clamps peer-advertised RSSI + * connection parameters to the locally legal range. The SYN/SYN-ACK handshake + * adopts the peer's advertised parameters verbatim; an out-of-range peer value + * (zero outstanding segments, zero timeouts, a sub-header-size segment, or a + * window larger than the locally offered maximum) would otherwise drive the + * controller into illegal state (for example curMaxBuffers == 0 makes the + * outbound wait loop spin forever). Conformant peers, including SLAC firmware, + * must be left untouched. + * ---------------------------------------------------------------------------- + * This file is part of the rogue software platform. It is subject to + * the license terms in the LICENSE.txt file found in the top-level directory + * of this distribution and at: + * https://confluence.slac.stanford.edu/display/ppareg/LICENSE.html. + * No part of the rogue software platform, including this file, may be + * copied, modified, propagated, or distributed except according to the terms + * contained in the LICENSE.txt file. + * ---------------------------------------------------------------------------- + **/ + +#include + +#include "doctest/doctest.h" +#include "rogue/protocols/rssi/Controller.h" +#include "rogue/protocols/rssi/Header.h" + +namespace rpr = rogue::protocols::rssi; + +// Local bounds used by the tests (rogue defaults: 32 buffers, 1400 byte segment) +static const uint8_t kLocMaxBuffers = 32; +static const uint16_t kLocMaxSegment = 1400; + +TEST_CASE("clampPeerParams leaves a conformant peer untouched") { + uint8_t maxBuffers = 8; + uint16_t maxSegment = 512; + uint16_t cumAckTout = 5; + uint16_t retranTout = 20; + uint16_t nullTout = 1000; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK_FALSE(clamped); + CHECK_EQ(maxBuffers, static_cast(8)); + CHECK_EQ(maxSegment, static_cast(512)); + CHECK_EQ(cumAckTout, static_cast(5)); + CHECK_EQ(retranTout, static_cast(20)); + CHECK_EQ(nullTout, static_cast(1000)); +} + +TEST_CASE("clampPeerParams raises a zero outstanding-segment count to 1") { + uint8_t maxBuffers = 0; + uint16_t maxSegment = 512; + uint16_t cumAckTout = 5; + uint16_t retranTout = 20; + uint16_t nullTout = 1000; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(maxBuffers, static_cast(1)); +} + +TEST_CASE("clampPeerParams caps an oversized window at the local maximum") { + uint8_t maxBuffers = 200; + uint16_t maxSegment = 512; + uint16_t cumAckTout = 5; + uint16_t retranTout = 20; + uint16_t nullTout = 1000; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(maxBuffers, kLocMaxBuffers); +} + +TEST_CASE("clampPeerParams raises a sub-header segment size to the header size") { + uint8_t maxBuffers = 8; + uint16_t maxSegment = 4; // smaller than the 8-byte header + uint16_t cumAckTout = 5; + uint16_t retranTout = 20; + uint16_t nullTout = 1000; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(maxSegment, static_cast(rpr::Header::HeaderSize)); +} + +TEST_CASE("clampPeerParams caps an oversized segment at the local maximum") { + uint8_t maxBuffers = 8; + uint16_t maxSegment = 9000; // larger than locally offered + uint16_t cumAckTout = 5; + uint16_t retranTout = 20; + uint16_t nullTout = 1000; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(maxSegment, kLocMaxSegment); +} + +TEST_CASE("clampPeerParams raises zero timeouts to 1") { + uint8_t maxBuffers = 8; + uint16_t maxSegment = 512; + uint16_t cumAckTout = 0; + uint16_t retranTout = 0; + uint16_t nullTout = 0; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(cumAckTout, static_cast(1)); + CHECK_EQ(retranTout, static_cast(1)); + CHECK_EQ(nullTout, static_cast(1)); +} + +TEST_CASE("clampPeerParams clamps multiple out-of-range fields at once") { + uint8_t maxBuffers = 0; + uint16_t maxSegment = 0; + uint16_t cumAckTout = 0; + uint16_t retranTout = 0; + uint16_t nullTout = 0; + + bool clamped = rpr::Controller::clampPeerParams(maxBuffers, + maxSegment, + cumAckTout, + retranTout, + nullTout, + kLocMaxBuffers, + kLocMaxSegment); + + CHECK(clamped); + CHECK_EQ(maxBuffers, static_cast(1)); + CHECK_EQ(maxSegment, static_cast(rpr::Header::HeaderSize)); + CHECK_EQ(cumAckTout, static_cast(1)); + CHECK_EQ(retranTout, static_cast(1)); + CHECK_EQ(nullTout, static_cast(1)); +}