Better handle TCP/IP stack crashes in the NETAPI compartment.

When the TCP/IP stack crashes, API calls to the compartment return
`-ECOMPARTMENTFAIL`.

Currently `-ECOMPARTMENTFAIL` failures are not considered by the NetAPI
compartment. This leads to erratic behavior when the network stack
crashes, such as adding nonsensical entries to the firewall.

Unfortunately these checks are really tricky to add in the case of
`socket_network_kind` and `network_host_resolve` because they return
structs.

This commit reworks the API so that these functions instead take a
pointer to a struct, and return an integer error code.

Building on this, add all the necessary checks.

Signed-off-by: Hugo Lefeuvre <[email protected]>

Author: hlef

lib/netapi/NetAPI.cc

@@ -55,18 +55,37 @@ SObj network_socket_connect_tcp(Timeout *timeout,
 		Debug::log("Host capability does not authorise a TCP connection");
 		return nullptr;
 	}
+
+	NetworkAddress    address{NetworkAddress::AddressKindInvalid};
+	CHERI::Capability addressPtr = &address;
+	addressPtr.permissions() &= {CHERI::Permission::Store};
 	firewall_permit_dns();
-	NetworkAddress address = network_host_resolve(host->hostname, UseIPv6);
+	int ret = network_host_resolve(host->hostname, UseIPv6, addressPtr);
 	firewall_permit_dns(false);
-	if (address.kind == NetworkAddress::AddressKindInvalid)
+	if ((ret < 0) || (address.kind == NetworkAddress::AddressKindInvalid))
 	{
 		Debug::log("Failed to resolve host");
 		return nullptr;
 	}
-	bool isIPv6       = address.kind == NetworkAddress::AddressKindIPv6;
-	auto sealedSocket = network_socket_create_and_bind(
+	bool isIPv6 = address.kind == NetworkAddress::AddressKindIPv6;
+
+	CHERI::Capability sealedSocket = network_socket_create_and_bind(
 	  timeout, mallocCapability, isIPv6, ConnectionTypeTCP);
-	auto kind = network_socket_kind(sealedSocket);
+	if (!sealedSocket.is_valid())
+	{
+		Debug::log("Failed to create socket");
+		return nullptr;
+	}
+
+	SocketKind        kind;
+	CHERI::Capability kindPtr = &kind;
+	kindPtr.permissions() &= {CHERI::Permission::Store};
+	if (network_socket_kind(sealedSocket, kindPtr) < 0)
+	{
+		Debug::log("Failed to retrieve socket kind");
+		return nullptr;
+	}
+
 	// FIXME: IPv6
 	if (isIPv6)
 	{
@@ -78,10 +97,13 @@ SObj network_socket_connect_tcp(Timeout *timeout,
 		firewall_add_tcpipv4_endpoint(
 		  address.ipv4, kind.localPort, ntohs(host->port));
 	}
+
 	if (network_socket_connect_tcp_internal(
 	      timeout, sealedSocket, address, host->port) != 0)
 	{
 		Timeout t{UnlimitedTimeout};
+		// We pass an unlimited timeout, so this cannot fail in any
+		// actionable manner. Don't check the return value.
 		network_socket_close(&t, mallocCapability, sealedSocket);
 		timeout->elapse(t.elapsed);
 		if (isIPv6)
@@ -118,7 +140,14 @@ NetworkAddress network_socket_udp_authorise_host(Timeout *timeout,
 		Debug::log("Host capability does not authorise a UDP connection");
 		return address;
 	}
-	auto kind   = network_socket_kind(socket);
+
+	SocketKind        kind;
+	CHERI::Capability kindPtr = &kind;
+	kindPtr.permissions() &= {CHERI::Permission::Store};
+	// No need to check the return value here, potential errors will be
+	// detected in the switch.
+	network_socket_kind(socket, kindPtr);
+
 	bool isIPv6 = false;
 	switch (kind.protocol)
 	{
@@ -133,10 +162,13 @@ NetworkAddress network_socket_udp_authorise_host(Timeout *timeout,
 			isIPv6 = true;
 			break;
 	}
+
+	CHERI::Capability addressPtr = &address;
+	addressPtr.permissions() &= {CHERI::Permission::Store};
 	firewall_permit_dns();
-	address = network_host_resolve(host->hostname, UseIPv6);
+	int ret = network_host_resolve(host->hostname, UseIPv6, addressPtr);
 	firewall_permit_dns(false);
-	if (address.kind == NetworkAddress::AddressKindInvalid)
+	if ((ret < 0) || (address.kind == NetworkAddress::AddressKindInvalid))
 	{
 		Debug::log("Failed to resolve host");
 		return address;
@@ -146,6 +178,7 @@ NetworkAddress network_socket_udp_authorise_host(Timeout *timeout,
 		Debug::log("Host address does not match socket type");
 		return address;
 	}
+
 	if (isIPv6)
 	{
 		firewall_add_udpipv6_endpoint(
@@ -161,6 +194,7 @@ NetworkAddress network_socket_udp_authorise_host(Timeout *timeout,
 		firewall_add_udpipv4_endpoint(
 		  address.ipv4, kind.localPort, ntohs(host->port));
 	}
+
 	return address;
 }
 

lib/tcpip/network-internal.h

@@ -17,9 +17,15 @@
  * Resolve a host name to an IPv4 or IPv6 address.  If `useIPv6` is true, then
  * this will first attempt to find a IPv6 address and fall back to IPv4 if none
  * is found.
+ *
+ * The result of the resolve is stored in `outAddress`.
+ *
+ * This returns zero for success, or a negative value on error.
  */
-NetworkAddress __cheri_compartment("TCPIP")
-  network_host_resolve(const char *hostname, bool useIPv6);
+__cheri_compartment("TCPIP") int network_host_resolve(
+  const char     *hostname,
+  bool            useIPv6,
+  NetworkAddress *outAddress);
 
 /**
  * Create a socket and bind it to the given address.  The socket will be
@@ -72,6 +78,9 @@ struct SocketKind
 };
 
 /**
- * Returns information about the given socket.
+ * Returns information about the given socket in `kind`.
+ *
+ * This returns zero for success, or a negative value on error.
  */
-SocketKind __cheri_compartment("TCPIP") network_socket_kind(SObj socket);
+int __cheri_compartment("TCPIP")
+  network_socket_kind(SObj socket, SocketKind *kind);

lib/tcpip/network_wrapper.cc

@@ -214,7 +214,8 @@ namespace
 	 * will favour IPv6 addresses, but can still return IPv4 addresses if no
 	 * IPv6 address is available.
 	 */
-	NetworkAddress host_resolve(const char *hostname, bool useIPv6 = UseIPv6)
+	int
+	host_resolve(const char *hostname, bool useIPv6, NetworkAddress *address)
 	{
 		struct AddrinfoHints hints;
 		hints.family = useIPv6 ? FREERTOS_AF_INET6 : FREERTOS_AF_INET;
@@ -229,31 +230,31 @@ namespace
 			// Try with IPv4 if the lookup failed with IPv6
 			if (useIPv6)
 			{
-				return host_resolve(hostname, false);
+				return host_resolve(hostname, false, address);
 			}
 			Debug::log("DNS request returned: {}", ret);
-			return {0, NetworkAddress::AddressKindInvalid};
+			address->kind = NetworkAddress::AddressKindInvalid;
+			address->ipv4 = 0;
+			return ret;
 		}
 
-		NetworkAddress address;
-		bool           isIPv6 = false;
+		bool isIPv6 = false;
 		for (freertos_addrinfo *r = results; r != nullptr; r = r->ai_next)
 		{
 			Debug::log("Canonical name: {}", r->ai_canonname);
 			if (r->ai_family == FREERTOS_AF_INET6)
 			{
 				memcpy(
-				  address.ipv6, r->ai_addr->sin_address.xIP_IPv6.ucBytes, 16);
-				address.kind = NetworkAddress::AddressKindIPv6;
+				  address->ipv6, r->ai_addr->sin_address.xIP_IPv6.ucBytes, 16);
+				address->kind = NetworkAddress::AddressKindIPv6;
 				Debug::log("Got IPv6 address");
 			}
 			else
 			{
-				address.ipv4 = r->ai_addr->sin_address.ulIP_IPv4;
-				address.kind = NetworkAddress::AddressKindIPv4;
-				Debug::log("Got IPv4 address");
+				address->ipv4 = r->ai_addr->sin_address.ulIP_IPv4;
+				address->kind = NetworkAddress::AddressKindIPv4;
 				Debug::log(
-				  "Got address: {}.{}.{}.{}",
+				  "Got IPv4 address: {}.{}.{}.{}",
 				  static_cast<int>(r->ai_addr->sin_address.ulIP_IPv4) & 0xff,
 				  static_cast<int>(r->ai_addr->sin_address.ulIP_IPv4) >> 8 &
 				    0xff,
@@ -265,7 +266,7 @@ namespace
 		}
 
 		FreeRTOS_freeaddrinfo(results);
-		return address;
+		return 0;
 	}
 
 	/**
@@ -422,9 +423,11 @@ namespace
 	}
 } // namespace
 
-NetworkAddress network_host_resolve(const char *hostname, bool useIPv6)
+int network_host_resolve(const char     *hostname,
+                         bool            useIPv6,
+                         NetworkAddress *address)
 {
-	return host_resolve(hostname, useIPv6);
+	return host_resolve(hostname, useIPv6, address);
 }
 
 SObj network_socket_create_and_bind(Timeout       *timeout,
@@ -959,27 +962,30 @@ ssize_t network_socket_send_to(Timeout              *timeout,
 	  socket);
 }
 
-SocketKind network_socket_kind(SObj socket)
+int network_socket_kind(SObj socket, SocketKind *kind)
 {
-	SocketKind kind = {SocketKind::Invalid, 0};
-	with_sealed_socket(
+	kind->protocol  = SocketKind::Invalid;
+	kind->localPort = 0;
+
+	int ret = with_sealed_socket(
 	  [&](SealedSocket *socket) {
 		  if (socket->socket->ucProtocol == FREERTOS_IPPROTO_TCP)
 		  {
-			  kind.protocol = socket->socket->bits.bIsIPv6
-			                    ? SocketKind::TCPIPv6
-			                    : SocketKind::TCPIPv4;
+			  kind->protocol = socket->socket->bits.bIsIPv6
+			                     ? SocketKind::TCPIPv6
+			                     : SocketKind::TCPIPv4;
 		  }
 		  else
 		  {
-			  kind.protocol = socket->socket->bits.bIsIPv6
-			                    ? SocketKind::UDPIPv6
-			                    : SocketKind::UDPIPv4;
+			  kind->protocol = socket->socket->bits.bIsIPv6
+			                     ? SocketKind::UDPIPv6
+			                     : SocketKind::UDPIPv4;
 		  }
-		  kind.localPort = listGET_LIST_ITEM_VALUE(
+		  kind->localPort = listGET_LIST_ITEM_VALUE(
 		    (&((socket->socket)->xBoundSocketListItem)));
 		  return 0;
 	  },
 	  socket);
-	return kind;
+
+	return ret;
 }