Defer remote commands outside receive stack

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, &timestamp, 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, &timestamp, 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;

examples/simple_repeater/MyMesh.h

@@ -61,6 +61,7 @@
 #include <helpers/ArduinoHelpers.h>
 #include <helpers/ClientACL.h>
 #include <helpers/CommonCLI.h>
+#include <helpers/DeferredCliCommand.h>
 #include <helpers/IdentityStore.h>
 #include <helpers/SimpleMeshTables.h>
 #include <helpers/StaticPoolPacketManager.h>
@@ -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);

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.

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

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++) {

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);
 

src/Nrf52LoopStack.cpp

@@ -0,0 +1,39 @@
+#if defined(NRF52_PLATFORM)
+
+#include <Arduino.h>
+#include <FreeRTOS.h>
+#include <task.h>
+#include <string.h>
+
+#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

src/helpers/DeferredCliCommand.h

@@ -0,0 +1,57 @@
+#pragma once
+
+#include <MeshCore.h>
+#include <stddef.h>
+#include <stdint.h>
+#include <string.h>
+
+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

test/test_deferred_cli_command/test_deferred_cli_command.cpp

@@ -0,0 +1,57 @@
+#include <gtest/gtest.h>
+
+#include <helpers/DeferredCliCommand.h>
+
+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();
+}