From c1752f2db143f170c00be11253dcd3ac72d0ceed Mon Sep 17 00:00:00 2001 From: mikecarper Date: Fri, 24 Jul 2026 00:18:35 -0700 Subject: [PATCH] Defer remote commands outside receive stack --- examples/simple_repeater/MyMesh.cpp | 126 ++++++++++++++---- examples/simple_repeater/MyMesh.h | 12 ++ examples/simple_repeater/main.cpp | 12 +- platformio.ini | 2 + src/Mesh.cpp | 3 + src/Mesh.h | 1 + src/Nrf52LoopStack.cpp | 39 ++++++ src/helpers/DeferredCliCommand.h | 57 ++++++++ .../test_deferred_cli_command.cpp | 57 ++++++++ 9 files changed, 281 insertions(+), 28 deletions(-) create mode 100644 src/Nrf52LoopStack.cpp create mode 100644 src/helpers/DeferredCliCommand.h create mode 100644 test/test_deferred_cli_command/test_deferred_cli_command.cpp diff --git a/examples/simple_repeater/MyMesh.cpp b/examples/simple_repeater/MyMesh.cpp index 2d6afeed..abb43617 100644 --- a/examples/simple_repeater/MyMesh.cpp +++ b/examples/simple_repeater/MyMesh.cpp @@ -760,11 +760,28 @@ static bool directPathsEqual(const uint8_t* a_path, uint8_t a_len, const uint8_t } void MyMesh::sendClientReply(ClientInfo* client, mesh::Packet* packet, unsigned long delay_millis, uint8_t path_hash_size) { + TransportKey fallback_scope; + const TransportKey* fallback_scope_ptr = NULL; + if (recv_pkt_region != NULL && !recv_pkt_region->isWildcard() + && region_map.getTransportKeysFor(*recv_pkt_region, &fallback_scope, 1) > 0) { + fallback_scope_ptr = &fallback_scope; + } + sendClientReplyWithFallbackScope(client, packet, delay_millis, path_hash_size, + fallback_scope_ptr); +} + +void MyMesh::sendClientReplyWithFallbackScope(ClientInfo* client, mesh::Packet* packet, + unsigned long delay_millis, uint8_t path_hash_size, + const TransportKey* fallback_scope) { if (packet == NULL) { return; } if (client == NULL || !mesh::Packet::isValidPathLen(client->out_path_len)) { - sendFloodReply(packet, delay_millis, path_hash_size); + if (fallback_scope != NULL) { + sendFloodScoped(*fallback_scope, packet, delay_millis, path_hash_size); + } else { + sendFlood(packet, delay_millis, path_hash_size); + } return; } @@ -2318,26 +2335,29 @@ void MyMesh::onPeerDataRecv(mesh::Packet *packet, uint8_t type, int sender_idx, sendClientReply(client, ack, TXT_ACK_DELAY, packet->getPathHashSize()); } - uint8_t temp[166]; char *command = (char *)&data[5]; - char *reply = (char *)&temp[5]; - if (is_retry) { - *reply = 0; - } else { - handleCommand(sender_timestamp, client, command, reply); - } - int text_len = strlen(reply); - if (text_len > 0) { - uint32_t timestamp = getRTCClock()->getCurrentTimeUnique(); - if (timestamp == sender_timestamp) { - // WORKAROUND: the two timestamps need to be different, in the CLI view - timestamp++; + if (!is_retry) { + TransportKey reply_scope; + const bool reply_scoped = + recv_pkt_region != NULL && !recv_pkt_region->isWildcard() + && region_map.getTransportKeysFor(*recv_pkt_region, &reply_scope, 1) > 0; + size_t command_len = strlen(command); + if (!deferred_cli_command.enqueue(i, sender_timestamp, packet->getPathHashSize(), + secret, command, command_len)) { + const char* error = deferred_cli_command.pending + ? "Err - another remote command is still running" + : "Err - remote command is too long"; + sendRemoteCliReply(client, secret, packet->getPathHashSize(), + sender_timestamp, error, + reply_scoped ? &reply_scope : NULL); + } else { + deferred_cli_reply_scoped = reply_scoped; + if (reply_scoped) { + deferred_cli_reply_scope = reply_scope; + } else { + memset(deferred_cli_reply_scope.key, 0, sizeof(deferred_cli_reply_scope.key)); + } } - memcpy(temp, ×tamp, 4); // mostly an extra blob to help make packet_hash unique - temp[4] = (TXT_TYPE_CLI_DATA << 2); // NOTE: legacy was: TXT_TYPE_PLAIN - - auto reply = createDatagram(PAYLOAD_TYPE_TXT_MSG, client->id, secret, temp, 5 + text_len); - sendClientReply(client, reply, CLI_REPLY_DELAY_MILLIS, packet->getPathHashSize()); } } else { MESH_DEBUG_PRINTLN("onPeerDataRecv: possible replay attack detected"); @@ -2345,6 +2365,59 @@ void MyMesh::onPeerDataRecv(mesh::Packet *packet, uint8_t type, int sender_idx, } } +void MyMesh::sendRemoteCliReply(ClientInfo* client, const uint8_t* secret, + uint8_t path_hash_size, uint32_t sender_timestamp, + const char* reply, const TransportKey* fallback_scope) { + if (client == NULL || secret == NULL || reply == NULL || reply[0] == 0) return; + + size_t text_len = strlen(reply); + const size_t max_text_len = + MAX_PACKET_PAYLOAD - CIPHER_MAC_SIZE - (CIPHER_BLOCK_SIZE - 1) - 5; + if (text_len > max_text_len) text_len = max_text_len; + + uint32_t timestamp = getRTCClock()->getCurrentTimeUnique(); + if (timestamp == sender_timestamp) { + // The two timestamps need to differ in the remote CLI view. + timestamp++; + } + memcpy(reply_data, ×tamp, sizeof(timestamp)); + reply_data[4] = (TXT_TYPE_CLI_DATA << 2); + if (reply != (const char*)&reply_data[5]) { + memmove(&reply_data[5], reply, text_len); + } + + mesh::Packet* packet = createDatagram(PAYLOAD_TYPE_TXT_MSG, client->id, secret, + reply_data, 5 + text_len); + sendClientReplyWithFallbackScope(client, packet, CLI_REPLY_DELAY_MILLIS, + path_hash_size, fallback_scope); +} + +void __attribute__((noinline)) MyMesh::processDeferredCliCommand() { + if (!deferred_cli_command.pending) return; + + const int client_index = deferred_cli_command.client_index; + if (client_index < 0 || client_index >= acl.getNumClients()) { + MESH_DEBUG_PRINTLN("processDeferredCliCommand: invalid client idx: %d", client_index); + deferred_cli_command.clear(); + deferred_cli_reply_scoped = false; + memset(deferred_cli_reply_scope.key, 0, sizeof(deferred_cli_reply_scope.key)); + return; + } + + ClientInfo* client = acl.getClientByIdx(client_index); + char* reply = (char*)&reply_data[5]; + reply[0] = 0; + handleCommand(deferred_cli_command.sender_timestamp, client, + deferred_cli_command.command, reply); + sendRemoteCliReply(client, deferred_cli_command.secret, + deferred_cli_command.path_hash_size, + deferred_cli_command.sender_timestamp, reply, + deferred_cli_reply_scoped ? &deferred_cli_reply_scope : NULL); + deferred_cli_command.clear(); + deferred_cli_reply_scoped = false; + memset(deferred_cli_reply_scope.key, 0, sizeof(deferred_cli_reply_scope.key)); +} + bool MyMesh::onPeerPathRecv(mesh::Packet *packet, int sender_idx, const uint8_t *secret, uint8_t *path, uint8_t path_len, uint8_t extra_type, uint8_t *extra, uint8_t extra_len) { // TODO: prevent replay attacks @@ -2461,6 +2534,8 @@ MyMesh::MyMesh(mesh::MainBoard &board, mesh::Radio &radio, mesh::MillisecondCloc last_millis = 0; uptime_millis = 0; next_local_advert = next_flood_advert = 0; + deferred_cli_reply_scoped = false; + memset(deferred_cli_reply_scope.key, 0, sizeof(deferred_cli_reply_scope.key)); pending_self_advert_delay = 0; pending_self_advert = false; pending_self_advert_flood = false; @@ -3655,11 +3730,8 @@ bool MyMesh::formatFileSystem() { } void MyMesh::sendSelfAdvertisement(int delay_millis, bool flood) { - // CommonCLI can invoke this while handling an encrypted remote-admin packet. - // Creating the advert signs it with Ed25519, whose large stack frame would - // otherwise sit on top of the entire RX/decrypt/command call chain and - // overflow the nRF52 Arduino loop task's 4 KiB stack. Defer the work until - // Mesh::loop() has returned and that inbound call chain has unwound. + // Keep signing outside command handlers and other callers. This is a second + // stack boundary in addition to deferred remote-command dispatch. pending_self_advert_delay = delay_millis > 0 ? (uint32_t)delay_millis : 0; pending_self_advert_flood = flood; pending_self_advert = true; @@ -7111,6 +7183,11 @@ void MyMesh::loop() { // Check radio FIRST to ensure we don't miss incoming packets // MQTT processing runs in a separate FreeRTOS task on Core 0, so we don't call bridge.loop() here mesh::Mesh::loop(); + processDeferredCliCommand(); + servicePostMeshLoop(); +} + +void __attribute__((noinline)) MyMesh::servicePostMeshLoop() { if (pending_self_advert) { const uint32_t delay_millis = pending_self_advert_delay; const bool flood = pending_self_advert_flood; @@ -7220,6 +7297,7 @@ void MyMesh::loop() { // To check if there is pending work bool MyMesh::hasPendingWork() const { + if (deferred_cli_command.pending || pending_self_advert) return true; #if defined(WITH_BRIDGE) const AbstractBridge* active_bridge = activeBridge(); if (active_bridge && active_bridge->isRunning()) return true; diff --git a/examples/simple_repeater/MyMesh.h b/examples/simple_repeater/MyMesh.h index 8edd7d95..70a0ff06 100644 --- a/examples/simple_repeater/MyMesh.h +++ b/examples/simple_repeater/MyMesh.h @@ -61,6 +61,7 @@ #include #include #include +#include #include #include #include @@ -204,6 +205,9 @@ class MyMesh : public mesh::Mesh, public CommonCLICallbacks uint32_t last_millis; uint64_t uptime_millis; unsigned long next_local_advert, next_flood_advert; + mesh::DeferredCliCommand deferred_cli_command; + TransportKey deferred_cli_reply_scope; + bool deferred_cli_reply_scoped; uint32_t pending_self_advert_delay; bool pending_self_advert; bool pending_self_advert_flood; @@ -405,6 +409,14 @@ class MyMesh : public mesh::Mesh, public CommonCLICallbacks uint8_t handleAnonClockReq(const mesh::Identity& sender, uint32_t sender_timestamp, const uint8_t* data); int handleRequest(ClientInfo* sender, uint32_t sender_timestamp, uint8_t* payload, size_t payload_len); mesh::Packet* createSelfAdvert(); + void processDeferredCliCommand(); + void sendRemoteCliReply(ClientInfo* client, const uint8_t* secret, + uint8_t path_hash_size, uint32_t sender_timestamp, + const char* reply, const TransportKey* fallback_scope); + void sendClientReplyWithFallbackScope(ClientInfo* client, mesh::Packet* packet, + unsigned long delay_millis, uint8_t path_hash_size, + const TransportKey* fallback_scope); + void servicePostMeshLoop(); void sendSelfAdvertisementNow(uint32_t delay_millis, bool flood); bool sendRepeatersFloodText(const char* text, const TransportKey* scope = nullptr, mesh::Packet** queued_packet = nullptr); diff --git a/examples/simple_repeater/main.cpp b/examples/simple_repeater/main.cpp index 9537ef07..982e2221 100644 --- a/examples/simple_repeater/main.cpp +++ b/examples/simple_repeater/main.cpp @@ -147,10 +147,7 @@ void setup() { board.onBootComplete(); } -void loop() { -#if defined(NRF52_PLATFORM) - board.feedWatchdog(the_mesh.getNodePrefs()->system_watchdog_enabled != 0); -#endif +static void __attribute__((noinline)) serviceCommandInterfaces() { // Handle Serial CLI int len = strlen(command); while (Serial.available() && len < sizeof(command)-1) { @@ -197,6 +194,13 @@ void loop() { ethernet_command[0] = 0; } #endif +} + +void loop() { +#if defined(NRF52_PLATFORM) + board.feedWatchdog(the_mesh.getNodePrefs()->system_watchdog_enabled != 0); +#endif + serviceCommandInterfaces(); #if defined(PIN_USER_BTN) && defined(_SEEED_SENSECAP_SOLAR_H_) && !defined(DISPLAY_CLASS) // Hold the user button to power off the SenseCAP Solar repeater. diff --git a/platformio.ini b/platformio.ini index 4309fc98..fbe5c207 100644 --- a/platformio.ini +++ b/platformio.ini @@ -112,6 +112,8 @@ build_flags = ${arduino_base.build_flags} -D NRF52_PLATFORM -D LFS_NO_ASSERT=1 -D EXTRAFS=1 + -D MESH_NRF52_LOOP_STACK_WORDS=2048 + -Wl,--wrap=xTaskCreate lib_deps = ${arduino_base.lib_deps} https://github.com/oltaco/CustomLFS#0.2.2 diff --git a/src/Mesh.cpp b/src/Mesh.cpp index 59a1777f..b3f5100a 100644 --- a/src/Mesh.cpp +++ b/src/Mesh.cpp @@ -218,7 +218,10 @@ void Mesh::begin() { void Mesh::loop() { Dispatcher::loop(); + serviceLoopMaintenance(); +} +void __attribute__((noinline)) Mesh::serviceLoopMaintenance() { if (_waiting_direct_retry_count != 0 && millisHasNowPassed(_next_direct_retry_timeout)) { for (int i = 0; i < MAX_DIRECT_RETRY_SLOTS; i++) { diff --git a/src/Mesh.h b/src/Mesh.h index dc6919a1..1fdaa4fc 100644 --- a/src/Mesh.h +++ b/src/Mesh.h @@ -149,6 +149,7 @@ class Mesh : public Dispatcher { void armFloodRetryOnSendComplete(const Packet* packet); void clearPendingFloodRetryOnSendFail(const Packet* packet); void maybeScheduleFloodRetry(const Packet* packet, uint8_t priority); + void serviceLoopMaintenance(); //void routeRecvAcks(Packet* packet, uint32_t delay_millis); DispatcherAction forwardMultipartDirect(Packet* pkt); diff --git a/src/Nrf52LoopStack.cpp b/src/Nrf52LoopStack.cpp new file mode 100644 index 00000000..3272e928 --- /dev/null +++ b/src/Nrf52LoopStack.cpp @@ -0,0 +1,39 @@ +#if defined(NRF52_PLATFORM) + +#include +#include +#include +#include + +#ifndef MESH_NRF52_LOOP_STACK_WORDS + #define MESH_NRF52_LOOP_STACK_WORDS 2048 +#endif + +static_assert(MESH_NRF52_LOOP_STACK_WORDS >= 1024, + "The nRF52 loop task stack must not be smaller than the framework default"); + +extern "C" BaseType_t __real_xTaskCreate( + TaskFunction_t task_code, const char* const task_name, + const configSTACK_DEPTH_TYPE stack_depth, void* const parameters, + UBaseType_t priority, TaskHandle_t* const created_task); + +// The Adafruit nRF52 core hard-codes the Arduino loop task to 1024 32-bit +// words (4 KiB). Mesh packet verification and authenticated remote commands +// can legitimately enter Ed25519 routines whose stack peaks do not fit there. +// Wrap task creation so only the framework's "loop" task receives 8 KiB; all +// BLE, timer, callback, and application-created tasks retain their requested +// sizes. +extern "C" BaseType_t __wrap_xTaskCreate( + TaskFunction_t task_code, const char* const task_name, + const configSTACK_DEPTH_TYPE stack_depth, void* const parameters, + UBaseType_t priority, TaskHandle_t* const created_task) { + configSTACK_DEPTH_TYPE adjusted_depth = stack_depth; + if (task_name != NULL && strcmp(task_name, "loop") == 0 + && stack_depth == 1024) { + adjusted_depth = MESH_NRF52_LOOP_STACK_WORDS; + } + return __real_xTaskCreate(task_code, task_name, adjusted_depth, parameters, + priority, created_task); +} + +#endif diff --git a/src/helpers/DeferredCliCommand.h b/src/helpers/DeferredCliCommand.h new file mode 100644 index 00000000..89ed8f42 --- /dev/null +++ b/src/helpers/DeferredCliCommand.h @@ -0,0 +1,57 @@ +#pragma once + +#include +#include +#include +#include + +namespace mesh { + +// A single-entry mailbox for authenticated remote CLI commands. The repeater +// drains it immediately after Mesh::loop() returns; a second command arriving +// in the same pass receives a busy response. Keeping the command in object +// storage lets the receive/decrypt call chain unwind before command handlers +// perform filesystem or cryptographic work. +struct DeferredCliCommand { + bool pending; + int client_index; + uint32_t sender_timestamp; + uint8_t path_hash_size; + uint8_t secret[PUB_KEY_SIZE]; + char command[MAX_PACKET_PAYLOAD + 1]; + + DeferredCliCommand() + : pending(false), client_index(-1), sender_timestamp(0), path_hash_size(1) { + memset(secret, 0, sizeof(secret)); + command[0] = 0; + } + + bool enqueue(int new_client_index, uint32_t new_sender_timestamp, + uint8_t new_path_hash_size, const uint8_t* new_secret, + const char* new_command, size_t command_len) { + if (pending || new_secret == NULL || new_command == NULL + || command_len >= sizeof(command)) { + return false; + } + + client_index = new_client_index; + sender_timestamp = new_sender_timestamp; + path_hash_size = new_path_hash_size; + memcpy(secret, new_secret, sizeof(secret)); + memcpy(command, new_command, command_len); + command[command_len] = 0; + pending = true; + return true; + } + + void clear() { + pending = false; + client_index = -1; + sender_timestamp = 0; + path_hash_size = 1; + memset(secret, 0, sizeof(secret)); + memset(command, 0, sizeof(command)); + } +}; + +} // namespace mesh diff --git a/test/test_deferred_cli_command/test_deferred_cli_command.cpp b/test/test_deferred_cli_command/test_deferred_cli_command.cpp new file mode 100644 index 00000000..ada72b04 --- /dev/null +++ b/test/test_deferred_cli_command/test_deferred_cli_command.cpp @@ -0,0 +1,57 @@ +#include + +#include + +TEST(DeferredCliCommand, CopiesAuthenticatedCommandContext) { + mesh::DeferredCliCommand deferred; + uint8_t secret[PUB_KEY_SIZE]; + memset(secret, 0x5A, sizeof(secret)); + const char command[] = "del flood.moderation.all"; + + ASSERT_TRUE(deferred.enqueue(7, 123456U, 2, secret, command, strlen(command))); + EXPECT_TRUE(deferred.pending); + EXPECT_EQ(7, deferred.client_index); + EXPECT_EQ(123456U, deferred.sender_timestamp); + EXPECT_EQ(2, deferred.path_hash_size); + EXPECT_EQ(0, memcmp(secret, deferred.secret, sizeof(secret))); + EXPECT_STREQ(command, deferred.command); + + secret[0] = 0; + EXPECT_EQ(0x5A, deferred.secret[0]); +} + +TEST(DeferredCliCommand, RejectsSecondCommandUntilCleared) { + mesh::DeferredCliCommand deferred; + uint8_t secret[PUB_KEY_SIZE] = {}; + const char first[] = "region save"; + const char second[] = "advert"; + + ASSERT_TRUE(deferred.enqueue(1, 10U, 1, secret, first, strlen(first))); + EXPECT_FALSE(deferred.enqueue(2, 11U, 3, secret, second, strlen(second))); + EXPECT_EQ(1, deferred.client_index); + EXPECT_STREQ(first, deferred.command); + + deferred.clear(); + EXPECT_FALSE(deferred.pending); + EXPECT_EQ(0, deferred.secret[0]); + EXPECT_EQ(0, deferred.command[0]); + ASSERT_TRUE(deferred.enqueue(2, 11U, 3, secret, second, strlen(second))); + EXPECT_STREQ(second, deferred.command); +} + +TEST(DeferredCliCommand, RejectsAnOverlongCommand) { + mesh::DeferredCliCommand deferred; + uint8_t secret[PUB_KEY_SIZE] = {}; + char command[MAX_PACKET_PAYLOAD + 2]; + memset(command, 'x', sizeof(command)); + command[sizeof(command) - 1] = 0; + + EXPECT_FALSE(deferred.enqueue(0, 1U, 1, secret, command, + sizeof(deferred.command))); + EXPECT_FALSE(deferred.pending); +} + +int main(int argc, char** argv) { + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}