Merge pull request #15 from CHERIoT-Platform/hlefeuvre/fix-dhcp

Three bug fixes in the TCP/IP stack.

Author: hlef

lib/firewall/firewall.cc

@@ -12,6 +12,31 @@
 #include <timeout.hh>
 #include <vector>
 
+namespace
+{
+	// TODO These should probably be in their own library.
+	uint16_t constexpr ntohs(uint16_t value)
+	{
+		return
+#ifdef __LITTLE_ENDIAN__
+		  __builtin_bswap16(value)
+#else
+		  value
+#endif
+		    ;
+	}
+	uint16_t constexpr htons(uint16_t value)
+	{
+		return
+#ifdef __LITTLE_ENDIAN__
+		  __builtin_bswap16(value)
+#else
+		  value
+#endif
+		    ;
+	}
+} // namespace
+
 namespace
 {
 	using Debug = ConditionalDebug<false, "Firewall">;
@@ -85,10 +110,13 @@ namespace
 		UDP  = 17,
 	};
 
+	static constexpr const uint16_t DhcpServerPort = 67;
+	static constexpr const uint16_t DhcpClientPort = 68;
+
 	struct IPv4Header
 	{
 		/**
-		 * Verson is in the low 4 bits, header length is in the high 4 bits.
+		 * Version is in the low 4 bits, header length is in the high 4 bits.
 		 */
 		uint8_t versionAndHeaderLength;
 		/**
@@ -286,6 +314,23 @@ namespace
 		}
 	};
 
+	bool is_dhcp_reply(enum IPProtocolNumber protocol,
+	                   bool                  isIngress,
+	                   uint16_t              remotePort,
+	                   uint16_t              localPort)
+	{
+		// A DHCP reply is an ingress UDP packet whose remote port
+		// matches the DHCP server port, and whose local port matches
+		// the DHCP client port.
+		if (isIngress && (protocol == IPProtocolNumber::UDP) &&
+		    (remotePort == htons(DhcpServerPort)) &&
+		    (localPort == htons(DhcpClientPort)))
+		{
+			return true;
+		}
+		return false;
+	}
+
 	uint32_t          dnsServerAddress;
 	_Atomic(uint32_t) dnsIsPermitted;
 
@@ -304,19 +349,13 @@ namespace
 		auto *ipv4Header = reinterpret_cast<const IPv4Header *>(data);
 		switch (ipv4Header->protocol)
 		{
-			// Drop all packets with unknown protocol types.
+			// Drop all packets with unknown IP protocol types.
 			default:
 				Debug::log("Dropping IPv4 packet with unknown protocol {}",
 				           ipv4Header->protocol);
 				return false;
 			case IPProtocolNumber::UDP:
-				// If we haven't finished doing DHCP, permit all UDP packets
-				// (we don't know the address of the DHCP server yet, so
-				// IP-based filtering is not going to be reliable).
-				if (__predict_false(dnsServerAddress == 0))
-				{
-					return true;
-				}
+
 				// Permit DNS requests during a DNS query.
 				if (dnsIsPermitted > 0)
 				{
@@ -356,6 +395,7 @@ namespace
 				uint32_t endpoint         = ipv4Header->*remoteAddress;
 				uint16_t localPortNumber  = tcpudpHeader->*localPort;
 				uint16_t remotePortNumber = tcpudpHeader->*remotePort;
+				bool isIngress = (remoteAddress == &IPv4Header::sourceAddress);
 				if (EndpointsTable<uint32_t>::instance().is_endpoint_permitted(
 				      ipv4Header->protocol,
 				      endpoint,
@@ -364,15 +404,21 @@ namespace
 				{
 					Debug::log("Permitting {} {} {}.{}.{}.{}",
 					           ipv4Header->protocol,
-					           remoteAddress == &IPv4Header::destinationAddress
-					             ? "to"
-					             : "from",
+					           isIngress ? "from" : "to",
 					           static_cast<int>(endpoint) & 0xff,
 					           static_cast<int>(endpoint >> 8) & 0xff,
 					           static_cast<int>(endpoint >> 16) & 0xff,
 					           static_cast<int>(endpoint >> 24) & 0xff);
 					return true;
 				}
+				// Permit DHCP replies
+				if (is_dhcp_reply(ipv4Header->protocol,
+				                  isIngress,
+				                  remotePortNumber,
+				                  localPortNumber))
+				{
+					return true;
+				}
 				return false;
 			}
 			break;

lib/sntp/xmake.lua

@@ -11,6 +11,6 @@ compartment("SNTP")
   add_deps("freestanding", "NetAPI")
   add_files("sntp.cc")
   add_includedirs(".", "../../include", "../../third_party/coreSNTP/source/include")
-  add_defines("CHERIOT_CUSTOM_DEFAULT_MALLOC_CAPABILITY")
+  add_cflags("-DCHERIOT_CUSTOM_DEFAULT_MALLOC_CAPABILITY")
   add_files("../../third_party/coreSNTP/source/core_sntp_client.c",
             "../../third_party/coreSNTP/source/core_sntp_serializer.c")

lib/tcpip/BufferManagement.cc

@@ -16,18 +16,19 @@ using Debug = ConditionalDebug<false, "Buffer management">;
 // 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
-// Size of the buffer manager allocator quota. Only relevant if
-// USE_DEDICATED_BUFFERMANAGER_POOL is enabled.
-#define BUFFER_MANAGER_QUOTA                                                   \
-	(ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS *                                  \
-	 (ipconfigTCP_MSS + ipBUFFER_PADDING))
 
 #if USE_DEDICATED_BUFFERMANAGER_POOL
+// Size of the buffer manager allocator quota.
+#	define BM_MALLOC_QUOTA                                                    \
+		(ipconfigNUM_NETWORK_BUFFER_DESCRIPTORS *                              \
+		 (ipconfigTCP_MSS + ipBUFFER_PADDING))
+// Separate allocator capability for the buffer manager.
 DECLARE_AND_DEFINE_ALLOCATOR_CAPABILITY(BufferManagementMallocQuota,
-                                        BUFFER_MANAGER_QUOTA);
-#	define BM_MALLOC_QUOTA STATIC_SEALED_VALUE(BufferManagementMallocQuota)
+                                        BM_MALLOC_QUOTA);
+#	define BM_MALLOC_CAPABILITY                                               \
+		STATIC_SEALED_VALUE(BufferManagementMallocQuota)
 #else
-#	define BM_MALLOC_QUOTA MALLOC_CAPABILITY
+#	define BM_MALLOC_CAPABILITY MALLOC_CAPABILITY
 #endif
 
 constexpr size_t MinimumBufferSize =
@@ -71,18 +72,18 @@ pxGetNetworkBufferWithDescriptor(size_t     xRequestedSizeBytes,
 
 	// TODO we likely want to pre-allocate (or re-use) the descriptors at
 	// some point
-	auto deleter = [=](void *ptr) { heap_free(BM_MALLOC_QUOTA, ptr); };
+	auto deleter = [=](void *ptr) { heap_free(BM_MALLOC_CAPABILITY, ptr); };
 	std::unique_ptr<NetworkBufferDescriptor_t, decltype(deleter)> descriptor{
-	  static_cast<NetworkBufferDescriptor_t *>(
-	    heap_allocate(&t, BM_MALLOC_QUOTA, sizeof(NetworkBufferDescriptor_t))),
+	  static_cast<NetworkBufferDescriptor_t *>(heap_allocate(
+	    &t, BM_MALLOC_CAPABILITY, sizeof(NetworkBufferDescriptor_t))),
 	  deleter};
 	if (descriptor == nullptr)
 	{
 		Debug::log("Failed to allocate descriptor");
 		return nullptr;
 	}
 	auto *buffer = static_cast<uint8_t *>(heap_allocate(
-	  &t, BM_MALLOC_QUOTA, xRequestedSizeBytes + ipBUFFER_PADDING));
+	  &t, BM_MALLOC_CAPABILITY, xRequestedSizeBytes + ipBUFFER_PADDING));
 	if (buffer == nullptr)
 	{
 		Debug::log("Failed to allocate {} byte buffer", xRequestedSizeBytes);
@@ -124,8 +125,8 @@ void vReleaseNetworkBufferAndDescriptor(
 		           networkBuffer,
 		           bufferWithoutOffset);
 
-		int ret = heap_free(MALLOC_CAPABILITY, bufferWithoutOffset);
-		ret |= heap_free(MALLOC_CAPABILITY, networkBuffer);
+		int ret = heap_free(BM_MALLOC_CAPABILITY, bufferWithoutOffset);
+		ret |= heap_free(BM_MALLOC_CAPABILITY, networkBuffer);
 
 		// Failure is not supposed to happen unless we have a bug here
 		// or in FreeRTOS.