Restart the TCP/IP stack on crash.

The TCP/IP stack currently has a lot of state and cannot recover
gracefully in case of failure. This commit addresses this limitation.

We introduce a custom error handler for the TCP/IP stack. At a
high-level, this error handler terminates all user threads currently
present in the TCP/IP compartment and resets the IP thread to its entry
point `ip_thread_entry`. It does so by setting all the locks of the
compartment for destruction and notifying all futex waiters. Then it
frees all heap allocations and resets globals. After all the state has
been cleaned up, it restarts the network stack.

Author: hlef

README.md

@@ -30,8 +30,8 @@ The new code in this repository is around 3% of the size of the large components
 Compartmentalisation
 --------------------
 
-The initial implementation has five compartments.
-Three are mostly existing third-party code with thin wrappers:
+The initial implementation has six compartments.
+Four are mostly existing third-party code with thin wrappers:
 
  - The TCP/IP stack is in a compartment.
    The FreeRTOS code was originally written on the assumption of a single security domain and separating it would require refactoring that would be hard to keep up to date.
@@ -75,14 +75,17 @@ graph TD
   classDef ThirdParty fill: #e44
 ```
 
-The TCP/IP stack has a lot of state and cannot yet recover gracefully in case of failure, but failure is at least isolated in the caller.
-In contrast, the TLS compartment is almost completely stateless.
-This gives strong flow isolation properties: Even if an attacker compromises the TLS compartment by sending malicious data over one connection that triggers a bug in BearSSL (unlikely), it is extraordinarily difficult for them to interfere with any other TLS connection.
+The TCP/IP stack is a large compartment with a lot of state.
+It is fault-tolerant: when an error is triggered (CHERI spatial or temporal safety fault, assertion), the compartment is automatically reset to a pristine state and restarted.
+We expand on this capability [below](#automatic-restart-of-the-tcpip-stack).
+
+Unlike the TCP/IP stack, the TLS compartment is almost completely stateless.
+This makes resetting the compartment trivial, and gives strong flow isolation properties: Even if an attacker compromises the TLS compartment by sending malicious data over one connection that triggers a bug in BearSSL (unlikely), it is extraordinarily difficult for them to interfere with any other TLS connection.
 
 Similarly, the firewall is controlled by the Network API compartment.
 The TCP/IP stack has no access to the control-plane interface for the compartment.
 A compromise that gets arbitrary-code execution in the network stack cannot open new firewall holes (to join a DDoS botnet such as [Mirai](https://en.wikipedia.org/wiki/Mirai_(malware)), for example).
-Note that there are currently some technical limitations to this, see the note below.
+Note that there are currently some technical limitations to this, see the discussion below.
 The worst it can do to rest of the system is provide malicious data, but a system using TLS will have HMACs on received messages and so this is no worse than a malicious packet being injected from the network.
 
 All of this is on top of the spatial and temporal safety properties that the CHERIoT platform provides at a base level.
@@ -224,3 +227,28 @@ Now that you know that the SNTP compartment is the only one that can send and re
 
 If you've modified the SNTP compartment to point to your NTP service and use its authentication credentials, then this should be different.
 This can all be part of your firmware's auditing policy.
+
+Automatic restart of the TCP/IP stack
+-------------------------------------
+
+We designed the TCP/IP stack to automatically and transparently restart on failure (e.g., a CHERI fault or an assertion).
+The restart procedure broadly works like this (simplified for didactic reasons):
+
+1. The error handler of the TCP/IP compartment is triggered and starts the reset procedure.
+2. It first sets a flag to prevent any new thread from entering the compartment.
+3. Then, it sets all synchronization primitives of the compartment (locks, futexes) for destruction. This wakes up sleeping threads present in the compartment, and prevents them from blocking again.
+4. Then, it waits for all threads present in the compartment (apart from the FreeRTOS network thread) to exit, either through normal control-flow, or by crashing.
+5. Finally, it frees all the memory of the compartment, resets all global state, and calls the start function of the network stack, which restarts the TCP/IP stack into a pristine working state.
+6. After the reset, any further call to the socket API (apart from `network_socket_close`) with an old socket from the previous instance of the network stack will be detected and failed with an `-ENOTCONN` code. This pushes callers to close the sockets and create new ones with the new instance of the TCP/IP stack.
+
+The implementation details of the reset slightly deviate from this description.
+See the technical documentation in `tcpip_error_handler.h` for a full perspective.
+
+Note that the current implementation of the automatic reset makes a few assumptions:
+
+- The TCP/IP stack cannot currently recover from a crash due to a stack overflow in the TCP/IP compartment. This is due to a limitation of the implementation of the switcher, which cannot trigger the error handler on stack overflow. This limitation should be addressed soon.
+- A small set of globals called 'reset-critical' outlive resets and/or are necessary for the reset. We assume that this data has not been corrupted. This data is correspondingly annotated in the source code.
+- The control-flow of threads in the compartment has not been altered.
+
+These assumptions leave some attack surface to malicious actors.
+We are working on improvements to remove or weaken them.

lib/firewall/firewall.cc

@@ -1,7 +1,7 @@
 // Copyright SCI Semiconductor and CHERIoT Contributors.
 // SPDX-License-Identifier: MIT
 
-#include "firewall.h"
+#include "firewall.hh"
 #include <atomic>
 #include <compartment-macros.h>
 #include <debug.hh>
@@ -69,6 +69,11 @@ namespace
 
 	std::atomic<uint32_t> barrier;
 
+	/**
+	 * This is used to synchronize with the TCP/IP stack during resets.
+	 */
+	std::atomic<uint8_t> *tcpipRestartState = nullptr;
+
 	auto &lazy_network_interface()
 	{
 		static EthernetDevice interface;
@@ -240,6 +245,13 @@ namespace
 			return table;
 		}
 
+		void clear(IPProtocolNumber protocol)
+		{
+			auto guardedTable = permitted_endpoints(protocol);
+			auto &[g, table]  = guardedTable;
+			table.clear();
+		}
+
 		void remove_endpoint(IPProtocolNumber protocol,
 		                     Address          endpoint,
 		                     uint16_t         localPort,
@@ -492,6 +504,16 @@ namespace
 
 	bool packet_filter_ingress(const uint8_t *data, size_t length)
 	{
+		uint32_t stateSnapshot = tcpipRestartState->load();
+		if (stateSnapshot != 0 &&
+		    ((stateSnapshot & RestartStateDriverKickedBit) == 0))
+		{
+			// We are in a reset and the driver has not yet been
+			// restarted.
+			Debug::log("Dropping packet due to network stack restart.");
+			return false;
+		}
+
 		// Not a valid Ethernet frame (64 bytes including four-byte FCS, which
 		// is stripped by this point).
 		if (length < 60)
@@ -527,26 +549,6 @@ namespace
 
 } // namespace
 
-bool ethernet_driver_start()
-{
-	// Protect against double entry.  If the barrier state is 0, no
-	// initialisation has happened and we should proceed.  If it's 1, we're in
-	// the middle of initialisation, if it's 2 then initialisation is done.  In
-	// any non-zero case, we should not try to do anything.
-	uint32_t expected = 0;
-	if (!barrier.compare_exchange_strong(expected, 1))
-	{
-		return false;
-	}
-	Debug::log("Initialising network interface");
-	auto &ethernet = lazy_network_interface();
-	ethernet.mac_address_set();
-	// Poke the barrier and make the driver thread start.
-	barrier = 2;
-	barrier.notify_one();
-	return true;
-}
-
 bool ethernet_send_frame(uint8_t *frame, size_t length)
 {
 	// We do not check the frame and length here, because we do not use it
@@ -748,3 +750,45 @@ void firewall_remove_udpipv6_remote_endpoint(uint8_t *remoteAddress,
 		  IPProtocolNumber::UDP, *copy, localPort, remotePort);
 	}
 }
+
+bool ethernet_driver_start(std::atomic<uint8_t> *state)
+{
+	if (tcpipRestartState == nullptr)
+	{
+		if (!CHERI::check_pointer<CHERI::PermissionSet{
+		      CHERI::Permission::Load, CHERI::Permission::Global}>(
+		      state, sizeof(*state)))
+		{
+			Debug::log("Invalid TCP/IP state pointer {}", state);
+			return false;
+		}
+		tcpipRestartState = state;
+	}
+	if (tcpipRestartState->load() != 0)
+	{
+		// This is a restart, no need to actually reset the driver.
+		// Instead, just remove all firewall entries.
+		Debug::log("Network stack restart: clearing all entries.");
+		EndpointsTable<IPv6Address>::instance().clear(IPProtocolNumber::UDP);
+		EndpointsTable<IPv6Address>::instance().clear(IPProtocolNumber::TCP);
+		EndpointsTable<uint32_t>::instance().clear(IPProtocolNumber::UDP);
+		EndpointsTable<uint32_t>::instance().clear(IPProtocolNumber::TCP);
+		return true;
+	}
+	// Protect against double entry.  If the barrier state is 0, no
+	// initialisation has happened and we should proceed.  If it's 1, we're in
+	// the middle of initialisation, if it's 2 then initialisation is done.  In
+	// any non-zero case, we should not try to do anything.
+	uint32_t expected = 0;
+	if (!barrier.compare_exchange_strong(expected, 1))
+	{
+		return false;
+	}
+	Debug::log("Initialising network interface");
+	auto &ethernet = lazy_network_interface();
+	ethernet.mac_address_set();
+	// Poke the barrier and make the driver thread start.
+	barrier = 2;
+	barrier.notify_one();
+	return true;
+}

lib/firewall/firewall.h renamed to lib/firewall/firewall.hh

@@ -1,6 +1,8 @@
 // Copyright SCI Semiconductor and CHERIoT Contributors.
 // SPDX-License-Identifier: MIT
 
+#pragma once
+#include <atomic>
 #include <compartment.h>
 
 /**
@@ -11,17 +13,31 @@ bool __cheri_compartment("Firewall")
   ethernet_send_frame(uint8_t *packet, size_t length);
 
 /**
- * Start the Firewall driver.  This returns true if the driver is successfully
- * started, false otherwise.  This should fail only if the driver is already
- * initialised.
+ * Start the Firewall driver.
+ *
+ * `state` should point to the reset state of the TCP/IP stack.
+ *
+ * This returns true if the driver is successfully started, false otherwise.
+ * This should fail only if the driver is already initialised (outside of a
+ * reset), or if `state` is invalid.
+ */
+bool __cheri_compartment("Firewall")
+  ethernet_driver_start(std::atomic<uint8_t> *state);
+
+/**
+ * Bit of the `state` atomic variable passed to `ethernet_driver_start` which
+ * indicates that the driver may send packets to the network stack.
+ *
+ * This must match the `DriverKicked` bit of enum `RestartState` (see
+ * `tcpip-internal.h`). This is statically asserted in the TCP/IP stack.
  */
-bool __cheri_compartment("Firewall") ethernet_driver_start(void);
+static constexpr uint32_t RestartStateDriverKickedBit = 0x4;
 
 /**
  * Query the link status of the Firewall driver.  This returns true if the link
  * is up, false otherwise.
  */
-bool __cheri_compartment("Firewall") ethernet_link_is_up(void);
+bool __cheri_compartment("Firewall") ethernet_link_is_up();
 
 /**
  * Receive a frame from the Firewall device via the on-device firewall.
@@ -42,7 +58,7 @@ void __cheri_compartment("Firewall") firewall_dns_server_ip_set(uint32_t ip);
  * This should be called only by the NetAPI compartment.
  */
 void __cheri_compartment("Firewall")
-  firewall_permit_dns(bool dnsIsPermitted __if_cxx(= true));
+  firewall_permit_dns(bool dnsIsPermitted = true);
 
 /**
  * Open a hole in the firewall for TCP packets to and from the given endpoint.

lib/netapi/NetAPI.cc

@@ -1,7 +1,7 @@
 // Copyright SCI Semiconductor and CHERIoT Contributors.
 // SPDX-License-Identifier: MIT
 
-#include "../firewall/firewall.h"
+#include "../firewall/firewall.hh"
 #include "../tcpip/network-internal.h"
 #include <NetAPI.h>
 

lib/tcpip/BufferManagement.cc

@@ -3,6 +3,7 @@
 
 #include "FreeRTOS.h"
 #include "FreeRTOS_IP.h"
+#include <BufferManagement.hh>
 #include <NetworkBufferManagement.h>
 #include <cstdlib>
 #include <debug.hh>
@@ -12,13 +13,6 @@ using Debug = ConditionalDebug<false, "Buffer management">;
 
 using CHERI::Capability;
 
-// Use a separate allocator quota for the buffer manager (false by default).
-// The buffer manager is responsible for allocating network buffers, which
-// differs from the other types of allocations the TCP/IP stack performs. It
-// may thus make sense to give it its own quota. We may want to expose this as
-// a build system configuration option at some point.
-#define USE_DEDICATED_BUFFERMANAGER_POOL false
-
 #if USE_DEDICATED_BUFFERMANAGER_POOL
 // Size of the buffer manager allocator quota.
 #	define BM_MALLOC_QUOTA                                                    \
@@ -29,6 +23,14 @@ DECLARE_AND_DEFINE_ALLOCATOR_CAPABILITY(BufferManagementMallocQuota,
                                         BM_MALLOC_QUOTA);
 #	define BM_MALLOC_CAPABILITY                                               \
 		STATIC_SEALED_VALUE(BufferManagementMallocQuota)
+
+/**
+ * Free the buffer manager's memory.
+ */
+void free_buffer_manager_memory()
+{
+	heap_free_all(BM_MALLOC_CAPABILITY);
+}
 #else
 #	define BM_MALLOC_CAPABILITY MALLOC_CAPABILITY
 #endif
@@ -132,7 +134,9 @@ void vReleaseNetworkBufferAndDescriptor(
 
 		// Failure is not supposed to happen unless we have a bug here
 		// or in FreeRTOS.
-		Debug::Assert(ret == 0, "Failed to free network buffer or descriptor.");
+		Debug::Assert(ret == 0,
+		              "Failed to free network buffer or descriptor (error {}).",
+		              ret);
 	}
 }
 

lib/tcpip/BufferManagement.hh

@@ -0,0 +1,31 @@
+// Copyright SCI Semiconductor and CHERIoT Contributors.
+// SPDX-License-Identifier: MIT
+
+#pragma once
+
+/**
+ * Internal helpers and configuration settings for use in the TCP/IP buffer
+ * manager. These should be called or used only from/in the TCP/IP compartment.
+ */
+
+#ifndef USE_DEDICATED_BUFFERMANAGER_POOL
+// Use a separate allocator quota for the buffer manager (false by default).
+// The buffer manager is responsible for allocating network buffers, which
+// differs from the other types of allocations the TCP/IP stack performs. It
+// may thus make sense to give it its own quota. We may want to expose this as
+// a build system configuration option at some point.
+#	define USE_DEDICATED_BUFFERMANAGER_POOL false
+#endif
+
+#if USE_DEDICATED_BUFFERMANAGER_POOL
+extern void free_buffer_manager_memory();
+#else
+/**
+ * Free the buffer manager's memory. Since the buffer manager does not have a
+ * dedicated quota, this is a noop.
+ */
+static inline void free_buffer_manager_memory()
+{
+	return;
+}
+#endif

lib/tcpip/FreeRTOSIPConfig.h

@@ -32,6 +32,28 @@
 #define configMAX_PRIORITIES 1
 #define configMINIMAL_STACK_SIZE 128
 
+/**
+ * FreeRTOS+TCP uses descriptors to track information about the packets in a
+ * window. A pool of descriptors is allocated when the first TCP connection is
+ * made. This configuration entry sets the size of the pool.
+ *
+ * In the worst case, each TCP connection will have an Rx and Tx window of at
+ * most 8 segments (according to the FreeRTOS+TCP documentation), requiring 16
+ * descriptors in total per TCP connection. This configuration entry should
+ * thus be set to `(expected maximum number of TCP connections) x 16`.
+ *
+ * If we assume at most 6 TCP connections, this results in 96 segments.
+ *
+ * Each descriptor is 104 bytes, i.e., set to 96 this results in a memory
+ * footprint of 9,984 bytes.
+ *
+ * Note that the pool of descriptors is reallocated at every network stack
+ * reset. Setting this number to a large value (e.g., 256, the FreeRTOS+TCP
+ * default) is known to cause reset failures due to fragmentation preventing us
+ * from reallocating the pool.
+ */
+#define ipconfigTCP_WIN_SEG_COUNT 96
+
 /**
  * Enable counting semaphore functionality in the build, as this is necessary
  * to build FreeRTOS+TCP.
@@ -63,7 +85,6 @@
 
 #define ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS 8
 
-
 #define ipconfigTCP_RX_BUFFER_LENGTH ( 1280 )
 #define ipconfigTCP_TX_BUFFER_LENGTH ( 1280 )
 
@@ -97,3 +118,10 @@
 
 #define ipconfigALLOW_SOCKET_SEND_WITHOUT_BIND 0
 #define ipconfigUSE_NETWORK_EVENT_HOOK 1
+
+// Necessary to have FreeRTOS+TCP invoke our own `ipFOREVER` function.  As of
+// FreeRTOS+TCP 3.1.0 this only enables external `ipFOREVER`. It would be nice
+// to upstream a patch to have this variable renamed into something specific to
+// `ipFOREVER` to ensure that we do not enable additional undesired options in
+// future releases.
+#define TEST