Fail type conversions on unexpected input bits

If a bitfield is passed to a type conversion function, but we encounter an unexpected bit, fail the conversion by default. This leaves it to the syscall layer to deal with explicitly masking off unexpected bits if they are ok to ignore. Add a flag allowing unexpected bits to be ignored for cases where the code knows it's ok to throw out unhandled bits. PiperOrigin-RevId: 365092363 Change-Id: I44e54f46ca3fcfb1a1b4bd37ef0160ea6b702aba
google · Mar 25, 2021 · 7985430 · 7985430
1 parent 81a275f
commit 7985430
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 48 deletions.
diff --git a/asylo/platform/host_call/serializer_functions.cc b/asylo/platform/host_call/serializer_functions.cc
@@ -112,6 +112,9 @@ bool IsIfAddrSupported(const struct ifaddrs *entry) {
   if (entry->ifa_ifu.ifu_dstaddr && !IpCompliant(entry->ifa_ifu.ifu_dstaddr)) {
     return false;
   }
+  if (!TokLinuxIffFlag(entry->ifa_flags, /*ignore_unexpected_bits=*/true)) {
+    return false;
+  }
   return true;
 }
 
@@ -222,7 +225,8 @@ bool DeserializeIfAddrs(primitives::MessageReader *in, struct ifaddrs **out,
     }
 
     Extent ifa_name_buf = in->next();
-    absl::optional<int> ifa_flags = FromkLinuxIffFlag(in->next<unsigned int>());
+    int klinux_flags = in->next<unsigned int>();
+    absl::optional<int> ifa_flags = FromkLinuxIffFlag(klinux_flags);
     Extent klinux_ifa_addr_buf = in->next();
     Extent klinux_ifa_netmask_buf = in->next();
     Extent klinux_ifa_dstaddr_buf = in->next();
@@ -374,16 +378,17 @@ PrimitiveStatus SerializeIfAddrs(primitives::MessageWriter *writer,
 
   for (struct ifaddrs *addr = ifaddr_list; addr != nullptr;
        addr = addr->ifa_next) {
-    absl::optional<uint32_t> ifa_flags = TokLinuxIffFlag(addr->ifa_flags);
-
     // If the entry is of a format we don't support, don't include it.
-    if (!IsIfAddrSupported(addr) || !ifa_flags) {
+    if (!IsIfAddrSupported(addr)) {
       continue;
     }
 
     // We push 5 entries per ifaddr to output.
     writer->PushString(addr->ifa_name);
-    writer->Push<uint32_t>(*ifa_flags);
+
+    // TokLinuxIffFlag was checked for validity in IsIfAddrSupported.
+    writer->Push<uint32_t>(
+        *TokLinuxIffFlag(addr->ifa_flags, /*ignore_unexpected_bits=*/true));
 
     if (!explicit_klinux_conversion) {
       ASYLO_RETURN_IF_ERROR(writer->PushSockAddr(addr->ifa_addr));

diff --git a/asylo/platform/host_call/test/host_call_test.cc b/asylo/platform/host_call/test/host_call_test.cc
@@ -681,13 +681,15 @@ TEST_F(HostCallTest, TestOpenExistingFile) {
       absl::StrCat(absl::GetFlag(FLAGS_test_tmpdir), "/test_file.tmp");
 
   int fd = open(path.c_str(), O_RDWR | O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR);
+  ASSERT_NE(fd, -1);
   ASSERT_THAT(write(fd, path.c_str(), path.length() + 1),
               Eq(path.length() + 1));
   ASSERT_THAT(access(path.c_str(), F_OK), Eq(0));
 
   MessageWriter in;
   in.PushString(path);
   in.Push<int>(/*value=flags*/ O_RDWR | O_CREAT | O_TRUNC);
+  in.Push<int>(/*value=mode*/ S_IRUSR | S_IWUSR);
 
   MessageReader out;
   ASYLO_ASSERT_OK(client_->EnclaveCall(kTestOpen, &in, &out));

diff --git a/asylo/platform/host_call/trusted/host_calls.cc b/asylo/platform/host_call/trusted/host_calls.cc
@@ -366,7 +366,8 @@ int enc_untrusted_fcntl(int fd, int cmd, ... /* arg */) {
     case F_GETFL: {
       int retval = EnsureInitializedAndDispatchSyscall(
           asylo::system_call::kSYS_fcntl, fd, *klinux_cmd, arg);
-      absl::optional<int> flags = FromkLinuxFileStatusFlag(retval);
+      absl::optional<int> flags =
+          FromkLinuxFileStatusFlag(retval, /*ignore_unexpected_bits=*/true);
       if (!flags) {
         errno = EINVAL;
         return -1;

diff --git a/asylo/platform/posix/io/io_context_epoll.cc b/asylo/platform/posix/io/io_context_epoll.cc
@@ -28,7 +28,7 @@ namespace asylo {
 namespace io {
 
 int IOContextEpoll::EpollCtl(int op, int hostfd, struct epoll_event *event) {
-  struct epoll_event event_copy;
+  struct epoll_event event_copy = {};
   if (event) {
     event_copy.events = event->events;
   }

diff --git a/asylo/platform/system_call/type_conversions/generated_types_functions_test.cc b/asylo/platform/system_call/type_conversions/generated_types_functions_test.cc
@@ -66,14 +66,24 @@ class GeneratedTypesFunctionsTest : public ::testing::Test {
   template <typename T>
   void TestMultiValuedEnums(
       const std::vector<T> &from_bits, const std::vector<int64_t> &to_bits,
-      std::function<absl::optional<int64_t>(int64_t)> from_function,
-      std::function<absl::optional<int64_t>(int64_t)> to_function) {
+      std::function<absl::optional<int64_t>(int64_t, bool)> from_function,
+      std::function<absl::optional<int64_t>(int64_t, bool)> to_function) {
     EXPECT_THAT(FuzzBitsetTranslationFunction(
                     from_bits, MakeOptionalVector(to_bits), kIterationCount),
                 IsFiniteRestrictionOf(from_function));
     EXPECT_THAT(FuzzBitsetTranslationFunction(
                     to_bits, MakeOptionalVector(from_bits), kIterationCount),
                 IsFiniteRestrictionOf(to_function));
+
+    // The above tests run with the default of "ignore_unexpected_bits" set to
+    // true. Add an extra quick check to ensure that if ALL bits are set, we
+    // still get a good conversion. Since the converters are automatically
+    // generated, and identical code, this should be a sufficient test of the
+    // ignore_unexpected_bits functionality.
+    EXPECT_NE(from_function(0xffffffff, /*ignore_unexpected_bits=*/true),
+              absl::nullopt);
+    EXPECT_NE(to_function(0xffffffff, /*ignore_unexpected_bits=*/true),
+              absl::nullopt);
   }
 
   // Tests that all values map both ways between |from| and |to|. Any values not

diff --git a/asylo/platform/system_call/type_conversions/types_conversions_generator.cc b/asylo/platform/system_call/type_conversions/types_conversions_generator.cc
@@ -27,6 +27,7 @@
 #include "absl/strings/str_cat.h"
 #include "absl/strings/str_format.h"
 #include "absl/strings/str_replace.h"
+#include "absl/strings/string_view.h"
 #include "asylo/util/logging.h"
 #include "asylo/platform/system_call/type_conversions/types_macros.inc"
 
@@ -101,23 +102,25 @@ std::string GetOrBasedEnumBody(bool to_prefix, const std::string &enum_name,
     if (enum_properties.wrap_macros_with_if_defined) {
       os << "#if defined(" << enum_pair.first << ")\n";
     }
-    os << "  if ((input & "
-       << (to_prefix ? enum_pair.first
-                     : absl::StrCat(klinux_prefix, "_", enum_pair.first))
-       << ") == "
-       << (to_prefix ? enum_pair.first
-                     : absl::StrCat(klinux_prefix, "_", enum_pair.first))
-       << ") output |= static_cast<" << enum_properties.data_type << ">("
-       << (to_prefix ? absl::StrCat(klinux_prefix, "_", enum_pair.first)
-                     : enum_pair.first)
-       << ");\n";
+    const std::string input_bit =
+        to_prefix ? enum_pair.first
+                  : absl::StrCat(klinux_prefix, "_", enum_pair.first);
+    const std::string output_bit =
+        to_prefix ? absl::StrCat(klinux_prefix, "_", enum_pair.first)
+                  : enum_pair.first;
+    os << "  if ((input & " << input_bit << ") == " << input_bit << ") {\n"
+       << "    output |= static_cast<" << enum_properties.data_type << ">("
+       << output_bit << ");\n";
+    // Mask off each handled bit so that we can later catch any unexpected bits.
+    os << "    input &= ~" << input_bit << ";\n"
+       << "  }\n";
     if (enum_properties.wrap_macros_with_if_defined) {
       os << "#endif\n";
     }
   }
 
-  os << "  if (output == 0) return absl::nullopt;\n"
-        "  return output;\n";
+  os << "  if (input != 0 && !ignore_unexpected_bits) return absl::nullopt;\n"
+     << "  return output;\n";
   return os.str();
 }
 
@@ -196,39 +199,45 @@ void WriteEnumConversions(
     const std::string return_data_type =
         absl::StrCat("absl::optional<", it.second.data_type, ">");
 
-    const absl::flat_hash_map<absl::string_view, absl::string_view> param_map =
-        {{"$klinux_prefix", klinux_prefix},
-         {"$enum_name", it.first},
-         {"$data_type", it.second.data_type},
-         {"$return_data_type", return_data_type}};
+    absl::flat_hash_map<absl::string_view, absl::string_view> param_map = {
+        {"$klinux_prefix", klinux_prefix},
+        {"$enum_name", it.first},
+        {"$data_type", it.second.data_type},
+        {"$return_data_type", return_data_type}};
 
-    std::string to_prefix_decl = absl::StrReplaceAll(
-        "$return_data_type To$klinux_prefix$enum_name($data_type input)",
-        param_map);
-    std::string from_prefix_decl = absl::StrReplaceAll(
-        "$return_data_type From$klinux_prefix$enum_name($data_type input)",
-        param_map);
+    const absl::string_view to_prefix_format =
+        "$return_data_type "
+        "To$klinux_prefix$enum_name($data_type input$optional_parameter)";
+    const absl::string_view from_prefix_format =
+        "$return_data_type "
+        "From$klinux_prefix$enum_name($data_type input$optional_parameter)";
 
     // Write the function declarations to the header file.
-    *os_h << "\n" << to_prefix_decl << "; \n";
-    *os_h << "\n" << from_prefix_decl << "; \n";
+    param_map["$optional_parameter"] =
+        it.second.multi_valued ? ", bool ignore_unexpected_bits=false" : "";
+    *os_h << "\n" << absl::StrReplaceAll(to_prefix_format, param_map) << "; \n";
+    *os_h << "\n"
+          << absl::StrReplaceAll(from_prefix_format, param_map) << "; \n";
 
     // Write the function body to the cc file.
+    std::string to_body;
+    std::string from_body;
     if (it.second.multi_valued) {
-      *os_cc << "\n"
-             << to_prefix_decl << " {\n"
-             << GetOrBasedEnumBody(true, enum_name_lower, it.second) << "}\n";
-      *os_cc << "\n"
-             << from_prefix_decl << " {\n"
-             << GetOrBasedEnumBody(false, enum_name_lower, it.second) << "}\n";
+      to_body = GetOrBasedEnumBody(true, enum_name_lower, it.second);
+      from_body = GetOrBasedEnumBody(false, enum_name_lower, it.second);
     } else {
-      *os_cc << "\n"
-             << to_prefix_decl << " {\n"
-             << GetIfBasedEnumBody(true, enum_name_lower, it.second) << "}\n";
-      *os_cc << "\n"
-             << from_prefix_decl << " {\n"
-             << GetIfBasedEnumBody(false, enum_name_lower, it.second) << "}\n";
+      to_body = GetIfBasedEnumBody(true, enum_name_lower, it.second);
+      from_body = GetIfBasedEnumBody(false, enum_name_lower, it.second);
     }
+
+    param_map["$optional_parameter"] =
+        it.second.multi_valued ? ", bool ignore_unexpected_bits" : "";
+    *os_cc << "\n"
+           << absl::StrReplaceAll(to_prefix_format, param_map) << " {\n"
+           << to_body << "}\n";
+    *os_cc << "\n"
+           << absl::StrReplaceAll(from_prefix_format, param_map) << " {\n"
+           << from_body << "}\n";
   }
 }
 

diff --git a/asylo/test/util/finite_domain_fuzz.cc b/asylo/test/util/finite_domain_fuzz.cc
@@ -74,7 +74,7 @@ FuzzBitsetTranslationFunction(
     }
     // Test multiple flags in and outside the defined domain
     in = 0;
-    out.reset();
+    out = 0;
     for (int j = 0; j < sizeof(int64_t) * 8 / 8; j++) {
       int flag = random_flag(bit_gen);
       auto found = std::find(input.begin(), input.end(), flag);
@@ -83,9 +83,13 @@ FuzzBitsetTranslationFunction(
       // output counterpart.
       if (found == input.end()) {
         in |= flag;
+        // A single invalid input bit means the output is not valid.
+        out = absl::nullopt;
       } else {
         in |= input[index];
-        out = (out ? *out : 0) | *output[index];
+        if (out) {
+          out = *out | *output[index];
+        }
       }
     }
     auto insert_result = all_cases.insert({in, out});

diff --git a/asylo/test/util/finite_domain_fuzz.h b/asylo/test/util/finite_domain_fuzz.h
@@ -73,6 +73,14 @@ PolymorphicMatcher<FiniteDomainMatcher<Domain, Range>> IsFiniteRestrictionOf(
   return MakePolymorphicMatcher(FiniteDomainMatcher<Domain, Range>(f));
 }
 
+template <typename Domain, typename Range>
+PolymorphicMatcher<FiniteDomainMatcher<Domain, Range>> IsFiniteRestrictionOf(
+    const std::function<Range(Domain, bool)>& f) {
+  std::function<Range(Domain)> bound =
+      std::bind(f, std::placeholders::_1, false);
+  return MakePolymorphicMatcher(FiniteDomainMatcher<Domain, Range>(bound));
+}
+
 template <typename T, typename U>
 absl::optional<std::vector<std::pair<T, U>>> zip(const std::vector<T>& ts,
                                                  const std::vector<U>& us) {