Merge pull request #46 from CHERIoT-Platform/hlefeuvre/bug-fixes

Various bug fixes in the network stack

include/NetAPI.h

@@ -219,8 +219,8 @@ int __cheri_compartment("TCPIP")
  * freeing this buffer.
  *
  * The `address` and `port` arguments are used to return the address and port
- * of the sender.  These can be null if the caller is not interested in the
- * sender's address or port.
+ * of the sender, in host byte order.  These can be null if the caller is not
+ * interested in the sender's address or port.
  *
  * The `bytesReceived` field of the result will be negative if an error
  * occurred.  The `buffer` field will be an untagged value if no data were
@@ -255,9 +255,9 @@ ssize_t __cheri_compartment("TCPIP") network_socket_send(Timeout *timeout,
 
 /**
  * Send data over a UDP socket to a specified host / port.  The address and
- * port must have been previously authorised with
- * `network_socket_udp_authorise_host` (the packets will be silently dropped if
- * not, there will be no error reported).
+ * port must be provided in host byte order, and must have been previously
+ * authorised with `network_socket_udp_authorise_host` (the packets will be
+ * silently dropped if not, there will be no error reported).
  *
  * This will block until the data have been sent or the timeout expires.  The
  * return value is the number of bytes sent or a negative error code.
@@ -370,7 +370,7 @@ struct BindCapability
 	DECLARE_AND_DEFINE_STATIC_SEALED_VALUE(                                    \
 	  struct {                                                                 \
 		  ConnectionType type;                                                 \
-		  short          port;                                                 \
+		  uint16_t       port;                                                 \
 		  size_t         nameLength;                                           \
 		  const char     hostname[sizeof(authorisedHost)];                     \
 	  },                                                                       \

include/endianness.hh

@@ -0,0 +1,26 @@
+// Copyright SCI Semiconductor and CHERIoT Contributors.
+// SPDX-License-Identifier: MIT
+
+#include <stdint.h>
+
+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
+	    ;
+}

include/mqtt.h

@@ -11,7 +11,9 @@
  * will be called on all PUBLISH notifications from the broker.
  *
  * `topicName` and `payload` (and their respective size arguments) indicate the
- * topic of the PUBLISH, and the corresponding payload.
+ * topic of the PUBLISH, and the corresponding payload. Both are only valid
+ * within the context of the callback and thus passed as a read-only,
+ * non-capturable capabilities.
  */
 typedef void __cheri_callback (*MQTTPublishCallback)(const char *topicName,
                                                      size_t topicNameLength,

include/tls.h

@@ -6,12 +6,13 @@
 #include <token.h>
 
 /**
- * Creates a new TLS connection.  Returns null on failure or a sealed TLS
- * connection object on success.
+ * Creates a new TLS connection. This will block until the connection is
+ * established, an error happens, or the timeout expires. Returns an untagged
+ * value on failure or a sealed TLS connection object on success.
  *
  * The state for the TLS connection will be allocated with `allocator`.  The
- * connection will be made to the host identified by the connection capability.
- * This must authorise a TCP connection.  Once the connection is made, the
+ * connection will be made to the host identified by the connection capability,
+ * which must authorise a TCP connection.  Once the connection is made, the
  * certificates will be validated against the trust anchors provided via the
  * `trustAnchors` parameter, which contains a pointer to an array of
  * `trustAnchorsCount` trust anchors.
@@ -57,13 +58,16 @@ enum TLSSendFlags
 };
 
 /**
- * Sends `length` bytes from `buffer` to the remote host.  Returns the number
- * of bytes sent, or a negative error code.  The `sealedConnection` parameter
- * is a pointer to a TLS connection, returned by `tls_connection_create`.
+ * Sends `length` bytes from `buffer` to the remote host. Returns the
+ * number of bytes sent, or a negative error code.
+ *
+ * The `sealedConnection` parameter is a pointer to a TLS connection, returned
+ * by `tls_connection_create`.
  *
  * If `flags` is set to `TLSSendNoFlush`, the TLS engine may buffer the data
  * and not send until a later send call.  If this is not provided then the
- * timeout may be exceeded.
+ * timeout may be exceeded. In the general case, this will block until the data
+ * is sent, an error happens, or the timeout expires.
  */
 ssize_t __cheri_compartment("TLS") tls_connection_send(Timeout *t,
                                                        SObj   sealedConnection,
@@ -72,10 +76,12 @@ ssize_t __cheri_compartment("TLS") tls_connection_send(Timeout *t,
                                                        int    flags);
 
 /**
- * Receive data from the TLS connection.  This returns a newly allocated buffer
- * (allocated with the allocator provided to `tls_connection_create`)
- * containing the received data along with its length, or null and a negative
- * error code.  The caller is responsible for freeing this buffer.
+ * Receive data from the TLS connection.  This will block until data are
+ * received, an error happens, or the timeout expires. If data are received,
+ * they will be stored in a newly allocated buffer (allocated with the
+ * allocator provided to `tls_connection_create`) and returned along with their
+ * length.  The caller is responsible for freeing this buffer. On error, the
+ * return value is an untagged value and a negative error code.
  *
  * The negative values will be errno values:
  *
@@ -88,8 +94,8 @@ NetworkReceiveResult __cheri_compartment("TLS")
 
 /**
  * Receive data from the TLS connection into a preallocated buffer. This will
- * block until data are received or the timeout expires. If data are received,
- * they will be stored in the provided buffer.
+ * block until data are received, an error happens, or the timeout expires. If
+ * data are received, they will be stored in the provided buffer.
  *
  * The return value is either the number of bytes received, zero if the
  * connection is closed, or a negative error code.

lib/firewall/firewall.cc

@@ -5,6 +5,7 @@
 #include <compartment-macros.h>
 #include <debug.hh>
 //#include <fail-simulator-on-error.h>
+#include <endianness.hh>
 #include <locks.hh>
 #include <platform-entropy.hh>
 #include <platform-ethernet.hh>
@@ -16,31 +17,6 @@ using Debug = ConditionalDebug<false, "Firewall">;
 
 #include "firewall.hh"
 
-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
 {
 

lib/firewall/firewall.hh

@@ -5,6 +5,11 @@
 #include <atomic>
 #include <compartment.h>
 
+/**
+ * Unless specified otherwise, all APIs exposed in this header take IP
+ * addresses and ports in network byte order.
+ */
+
 /**
  * Send a frame through the on-device firewall.  This returns true if the
  * packet is successfully sent, false otherwise.

lib/mqtt/mqtt.cc

@@ -468,9 +468,17 @@ namespace
 			              "The packet is of type PUBLISH, but topic or payload "
 			              "are not set.");
 
-			publishCallback(publishInfo->pTopicName,
+			// The payload and topic are only valid within the
+			// context of the callback: make them read-only and
+			// non-capturable.
+			Capability topic{publishInfo->pTopicName};
+			Capability payload{publishInfo->pPayload};
+			topic.permissions() &= CHERI::Permission::Load;
+			payload.permissions() &= CHERI::Permission::Load;
+
+			publishCallback(topic,
 			                publishInfo->topicNameLength,
-			                publishInfo->pPayload,
+			                payload,
 			                publishInfo->payloadLength);
 		}
 		else if (ackCallback)

lib/netapi/NetAPI.cc

@@ -6,6 +6,7 @@
 
 #include <atomic>
 #include <debug.hh>
+#include <endianness.hh>
 #include <token.h>
 
 using Debug = ConditionalDebug<false, "Network API">;
@@ -14,19 +15,6 @@ using Debug = ConditionalDebug<false, "Network API">;
 
 namespace
 {
-	uint16_t ntohs(uint16_t value)
-	{
-		return __builtin_bswap16(value);
-	}
-	uint16_t htons(uint16_t value)
-	{
-		return __builtin_bswap16(value);
-	}
-} // namespace
-
-namespace
-{
-	using Debug            = ConditionalDebug<false, "Network API">;
 	constexpr bool UseIPv6 = CHERIOT_RTOS_OPTION_IPv6;
 
 	/**

lib/tcpip/network-internal.h

@@ -46,7 +46,7 @@ __cheri_compartment("TCPIP") int network_host_resolve(
  *
  * This returns a sealed capability to a socket on success, or null on failure.
  *
- * This should be called only from the NetAPI compartment.
+ * This should be called only from the NetAPI or TCP/IP compartments.
  */
 SObj __cheri_compartment("TCPIP")
   network_socket_create_and_bind(Timeout       *timeout,

lib/tcpip/network_wrapper.cc

@@ -12,6 +12,7 @@
 #include <FreeRTOS_IP_Private.h>
 #include <NetAPI.h>
 #include <debug.hh>
+#include <endianness.hh>
 #include <function_wrapper.hh>
 #include <limits>
 #include <locks.hh>
@@ -73,28 +74,6 @@ using CHERI::PermissionSet;
 
 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
-		    ;
-	}
-
 	/**
 	 * Returns the key with which SealedSocket instances are sealed.
 	 */
@@ -631,7 +610,7 @@ SObj network_socket_create_and_bind(Timeout       *timeout,
 			  // or passing pxAddress as NULL will result in the socket being
 			  // bound to a port number from the private range'. Here,
 			  // `localPort` will be 0 if the caller wants to bind to any port.
-			  localAddress.sin_port   = FreeRTOS_htons(localPort);
+			  localAddress.sin_port   = htons(localPort);
 			  localAddress.sin_family = Family;
 			  auto bindResult =
 			    FreeRTOS_bind(socket, &localAddress, sizeof(localAddress));
@@ -831,7 +810,7 @@ int network_socket_connect_tcp_internal(Timeout       *timeout,
 		  struct freertos_sockaddr server;
 		  memset(&server, 0, sizeof(server));
 		  server.sin_len  = sizeof(server);
-		  server.sin_port = FreeRTOS_htons(port);
+		  server.sin_port = htons(port);
 		  if (isIPv6)
 		  {
 			  server.sin_family = FREERTOS_AF_INET6;
@@ -923,7 +902,7 @@ int network_socket_close(Timeout *t, SObj mallocCapability, SObj sealedSocket)
 				  // happen in practice and has no impact here.
 				  FreeRTOS_shutdown(rawSocket, FREERTOS_SHUT_RDWR);
 
-				  auto localPort = ntohs(rawSocket->usLocalPort);
+				  auto localPort = htons(rawSocket->usLocalPort);
 				  if (rawSocket->bits.bIsIPv6)
 				  {
 					  if (isTCP)
@@ -1303,7 +1282,7 @@ ssize_t network_socket_send_to(Timeout              *timeout,
 		  struct freertos_sockaddr server;
 		  memset(&server, 0, sizeof(server));
 		  server.sin_len  = sizeof(server);
-		  server.sin_port = FreeRTOS_htons(port);
+		  server.sin_port = htons(port);
 		  if (heap_claim_fast(timeout, address) < 0)
 		  {
 			  Debug::log("Failed to claim address");

lib/tcpip/tcpip_error_handler.h

@@ -232,8 +232,8 @@ extern "C" void reset_network_stack_state()
 
 	// Upgrade the message queue lock for destruction
 	DebugErrorHandler::log("Upgrading the message queue for destruction.");
-	auto *queueHandle = &xNetworkEventQueue->handle;
-	if (int err = queue_destroy(MALLOC_CAPABILITY, queueHandle); err != 0)
+	if (int err = queue_destroy(MALLOC_CAPABILITY, xNetworkEventQueue);
+	    err != 0)
 	{
 		DebugErrorHandler::log(
 		  "Failed to upgrade the message queue for destruction (error {}).",

network_stack.rego

@@ -164,7 +164,7 @@ valid(ethernetDevice) {
 	firewall_thread_is_valid
 	network_thread_is_valid
 	data.compartment.compartment_call_allow_list("TCPIP", "network_host_resolve.*", {"NetAPI"})
-	data.compartment.compartment_call_allow_list("TCPIP", "network_socket_create_and_bind.*", {"NetAPI"})
+	data.compartment.compartment_call_allow_list("TCPIP", "network_socket_create_and_bind.*", {"NetAPI", "TCPIP"})
 	data.compartment.compartment_call_allow_list("TCPIP", "network_socket_connect_tcp_internal.*", {"NetAPI"})
 	data.compartment.compartment_call_allow_list("TCPIP", "ethernet_receive_frame.*", {"Firewall"})
 	data.compartment.compartment_call_allow_list("TCPIP", "ip_thread_entry.*", set())