Merge "Properly handle unsetting of override" into main am: 24698eaa79 am: d649c3e6b5 am: cb11192484

Original change: https://android-review.googlesource.com/c/platform/system/extras/+/2657256

Change-Id: Iff1a499175e6420ed25edc20974d32cf49afc2d0
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

2 files changed, 215 insertions, 39 deletions

diff --git a/mtectrl/mtectrl.cc b/mtectrl/mtectrl.cc
index 0d862511..3032ad5f 100644
--- a/mtectrl/mtectrl.cc
+++ b/mtectrl/mtectrl.cc
@@ -47,6 +47,7 @@ bool UpdateProp(const char* prop_name, const misc_memtag_message& m) {
   if (CheckAndUnset(mode, MISC_MEMTAG_MODE_MEMTAG_KERNEL_ONCE))
     AddItem(&prop_str, "memtag-kernel-once");
   if (CheckAndUnset(mode, MISC_MEMTAG_MODE_MEMTAG_OFF)) AddItem(&prop_str, "memtag-off");
+  if (CheckAndUnset(mode, MISC_MEMTAG_MODE_FORCED)) AddItem(&prop_str, "forced");
   if (prop_str.empty()) prop_str = "none";
   if (android::base::GetProperty(prop_name, "") != prop_str)
     android::base::SetProperty(prop_name, prop_str);
@@ -94,6 +95,9 @@ void PrintUsage(const char* progname) {
          "        - memtag-kernel-once: MTE is enabled in the kernel, only for the next reboot.\n"
          "        - memtag-off: MTE is persistently disabled in both userspace and kernel upon \n"
          "                      the next reboot.\n"
+         "        - forced: the current state is the result of force_on or force_off in the next\n"
+         "                  argument. When the next argument is set back to \"default\", the\n"
+         "                  state will be cleared.\n"
          "  [default|force_on|force_off]\n"
          "      An alternative method of configuring the MTE options to be applied, if provided.\n"
          "      This control is generally to be used by device_config only, and it overwrites\n"
@@ -121,6 +125,8 @@ int StringToMode(const char* value) {
       memtag_mode |= MISC_MEMTAG_MODE_MEMTAG_KERNEL_ONCE;
     } else if (field == "memtag-off") {
       memtag_mode |= MISC_MEMTAG_MODE_MEMTAG_OFF;
+    } else if (field == "forced") {
+      memtag_mode |= MISC_MEMTAG_MODE_FORCED;
     } else if (field != "none") {
       LOG(ERROR) << "Unknown value for mode: " << field;
       return -1;
@@ -129,15 +135,103 @@ int StringToMode(const char* value) {
   return memtag_mode;
 }
 
+// Handles the override flag and applies it to the memtag message.
+// The logic is as follows:
+// If the override changes the configuration (i.e., if MTE was not enabled
+// through MODE_MEMTAG and the override is force_on, or MTE was not
+// disabled through MEMTAG_OFF and the override is force_off), the MTE
+// state is considered FORCED. In that case, if the override gets reset
+// to "default" (i.e. no override), the default state of memtag config
+// is restored. The theory for this is that disabling the override should
+// only keep the non-default state if it has been active throughout the
+// override, not restore it if it had been dormant for the duration of the
+// override.
+//
+// State machine diagrams of the MTE state and the effect of override below:
+//
+//                      default,force_off
+//                           ┌───┐
+//                           │   │
+//                        ┌──┴───▼───┐
+//                        │memtag-off│
+//                        └─────┬────┘
+//                              │
+//                     force_on │   ┌────┐
+//                              │   │    │ force_on
+//             force_off┌───────▼───┴─┐  │
+//             ┌────────┤memtag,forced│◄─┘
+//             │        └▲─────────┬──┘
+// force_off   │         │         │
+//   ┌────┐    │ force_on│         │ default
+//   │    │    │         │         │
+//   │  ┌─┴────▼─────────┴┐       ┌▼──────┐
+//   └─►│memtag-off,forced├───────►none   │
+//      └─────────────────┘default└───────┘
+//
+//
+//
+//                      default,force_on
+//                           ┌───┐
+//                           │   │
+//                        ┌──┴───▼───┐
+//                        │memtag    │
+//                        └─────┬────┘
+//                              │
+//                     force_off│       ┌────┐
+//                              │       │    │ force_off
+//             force_on ┌───────┴───────┴─┐  │
+//             ┌────────┤memtag-off,forced◄──┘
+//             │        └▲─────────┬──────┘
+// force_on    │         │         │
+//   ┌────┐    │force_off│         │ default
+//   │    │    │         │         │
+//   │  ┌─┴────▼─────────┴┐       ┌▼──────┐
+//   └─►│memtag,forced    ├───────►none   │
+//      └─────────────────┘default└───────┘
+//
+//
+//
+//                           default
+//                            ┌───┐
+//                            │   │
+//              force_off  ┌──┴───▼───┐
+//           ┌─────────────┤none      │
+//           │             └─────┬────┘
+//           │                   │
+//           │          force_on │   ┌────┐
+//           │                   │   │    │ force_on
+//           │  force_off┌───────▼───┴─┐  │
+//           │  ┌────────┤memtag,forced│◄─┘
+//           │  │        └▲─────────┬──┘
+//  force_off│  │         │         │
+//    ┌────┐ │  │ force_on│         │ default
+//    │    │ │  │         │         │
+//    │  ┌─┴─▼──▼─────────┴┐       ┌▼──────┐
+//    └─►│memtag-off,forced├───────►none   │
+//       └─────────────────┘default└───────┘
 bool HandleOverride(const std::string& override_value, misc_memtag_message* m) {
   if (override_value == "force_off") {
     // If the force_off override is active, only allow MEMTAG_MODE_MEMTAG_ONCE.
+    if ((m->memtag_mode & MISC_MEMTAG_MODE_MEMTAG_OFF) == 0) {
+      m->memtag_mode |= MISC_MEMTAG_MODE_FORCED;
+    }
     m->memtag_mode |= MISC_MEMTAG_MODE_MEMTAG_OFF;
     m->memtag_mode &= ~MISC_MEMTAG_MODE_MEMTAG;
   } else if (override_value == "force_on") {
+    if ((m->memtag_mode & MISC_MEMTAG_MODE_MEMTAG) == 0) {
+      m->memtag_mode |= MISC_MEMTAG_MODE_FORCED;
+    }
     m->memtag_mode |= MISC_MEMTAG_MODE_MEMTAG;
     m->memtag_mode &= ~MISC_MEMTAG_MODE_MEMTAG_OFF;
-  } else if (!override_value.empty() && override_value != "default") {
+  } else if (override_value.empty() || override_value == "default") {
+    // The mode changed from forced_on or forced_off to default, which means we
+    // restore the normal state.
+    if (m->memtag_mode & MISC_MEMTAG_MODE_FORCED) {
+      m->memtag_mode &= ~MISC_MEMTAG_MODE_MEMTAG;
+      m->memtag_mode &= ~MISC_MEMTAG_MODE_MEMTAG_OFF;
+      m->memtag_mode &= ~MISC_MEMTAG_MODE_FORCED;
+    }
+  } else {
     return false;
   }
   return true;
diff --git a/mtectrl/mtectrl_test.cc b/mtectrl/mtectrl_test.cc
index d9f3c370..d4463cb3 100644
--- a/mtectrl/mtectrl_test.cc
+++ b/mtectrl/mtectrl_test.cc
@@ -23,16 +23,55 @@
 #include <android-base/logging.h>
 #include <android-base/properties.h>
 #include <bootloader_message/bootloader_message.h>
+#include <string_view>
 
 namespace {
 using ::testing::StartsWith;
 
-int mtectrl(const char* arg) {
+int mtectrl(std::string_view arg) {
   std::string cmd = "mtectrl -t /data/local/tmp/misc_memtag ";
   cmd += arg;
   return system(cmd.c_str());
 }
 
+int RunMteCtrl() {
+  CHECK(android::base::GetIntProperty("arm64.memtag.test_bootctl_loaded", 0) == 1);
+  std::string arg = android::base::GetProperty("arm64.memtag.test_bootctl", "none");
+  arg += " ";
+  arg += android::base::GetProperty("arm64.memtag.test_bootctl_override", "default");
+  return mtectrl(arg);
+}
+
+void Boot(misc_memtag_message m) {
+  std::string m_str(reinterpret_cast<char*>(&m), sizeof(m));
+  android::base::WriteStringToFile(m_str, "/data/local/tmp/misc_memtag");
+  mtectrl("-s arm64.memtag.test_bootctl -f arm64.memtag.test_bootctl_loaded");
+  // arm64.memtag.test_bootctl got updated, so we trigger ourselves.
+  RunMteCtrl();
+}
+
+void Reboot() {
+  android::base::SetProperty("arm64.memtag.test_bootctl", "INVALID");
+  android::base::SetProperty("arm64.memtag.test_bootctl_loaded", "0");
+  std::string m_str;
+  ASSERT_TRUE(android::base::ReadFileToString("/data/local/tmp/misc_memtag", &m_str));
+  misc_memtag_message m;
+  ASSERT_EQ(m_str.size(), sizeof(m));
+  memcpy(&m, m_str.c_str(), sizeof(m));
+  m.memtag_mode &= ~MISC_MEMTAG_MODE_MEMTAG_ONCE;
+  Boot(m);
+}
+
+void SetMemtagProp(const std::string& s) {
+  android::base::SetProperty("arm64.memtag.test_bootctl", s);
+  RunMteCtrl();
+}
+
+void SetOverrideProp(const std::string& s) {
+  android::base::SetProperty("arm64.memtag.test_bootctl_override", s);
+  RunMteCtrl();
+}
+
 std::string GetMisc() {
   std::string data;
   CHECK(android::base::ReadFileToString("/data/local/tmp/misc_memtag", &data, false));
@@ -55,6 +94,7 @@ class MteCtrlTest : public ::testing::Test {
     CHECK(ftruncate(fd, sizeof(misc_memtag_message)) != -1);
     close(fd);
     android::base::SetProperty("arm64.memtag.test_bootctl", "INVALID");
+    android::base::SetProperty("arm64.memtag.test_bootctl_override", "");
     android::base::SetProperty("arm64.memtag.test_bootctl_loaded", "0");
   }
   void TearDown() override {
@@ -68,37 +108,28 @@ TEST_F(MteCtrlTest, invalid) {
 }
 
 TEST_F(MteCtrlTest, set_once) {
-  ASSERT_EQ(mtectrl("memtag-once"), 0);
+  Boot({});
+  SetMemtagProp("memtag-once");
   EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x02"));
 }
 
 TEST_F(MteCtrlTest, set_once_kernel) {
-  ASSERT_EQ(mtectrl("memtag-once,memtag-kernel"), 0);
+  Boot({});
+  SetMemtagProp("memtag-once,memtag-kernel");
   EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x06"));
 }
 
-TEST_F(MteCtrlTest, set_memtag) {
-  ASSERT_EQ(mtectrl("memtag"), 0);
-  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x01"));
-}
-
-TEST_F(MteCtrlTest, set_memtag_force_off) {
-  ASSERT_EQ(mtectrl("memtag force_off"), 0);
-  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x10"));
-}
-
 TEST_F(MteCtrlTest, read_memtag) {
-  ASSERT_EQ(mtectrl("memtag"), 0);
-  ASSERT_EQ(mtectrl("-s arm64.memtag.test_bootctl -f arm64.memtag.test_bootctl_loaded"), 0);
+  Boot({});
+  SetMemtagProp("memtag");
+  Reboot();
   EXPECT_EQ(TestProperty(), "memtag");
   EXPECT_EQ(TestFlag(), "1");
 }
 
 TEST_F(MteCtrlTest, read_invalid_memtag_message) {
   misc_memtag_message m = {.version = 1, .magic = 0xffff, .memtag_mode = MISC_MEMTAG_MODE_MEMTAG};
-  std::string m_str(reinterpret_cast<char*>(&m), sizeof(m));
-  android::base::WriteStringToFile(m_str, "/data/local/tmp/misc_memtag");
-  ASSERT_EQ(mtectrl("-s arm64.memtag.test_bootctl -f arm64.memtag.test_bootctl_loaded"), 0);
+  Boot(m);
   EXPECT_EQ(TestProperty(), "none");
   EXPECT_EQ(TestFlag(), "1");
 }
@@ -107,49 +138,100 @@ TEST_F(MteCtrlTest, read_invalid_memtag_mode) {
   misc_memtag_message m = {.version = MISC_MEMTAG_MESSAGE_VERSION,
                            .magic = MISC_MEMTAG_MAGIC_HEADER,
                            .memtag_mode = MISC_MEMTAG_MODE_MEMTAG | 1u << 31};
-  std::string m_str(reinterpret_cast<char*>(&m), sizeof(m));
-  android::base::WriteStringToFile(m_str, "/data/local/tmp/misc_memtag");
-  ASSERT_NE(mtectrl("-s arm64.memtag.test_bootctl -f arm64.memtag.test_bootctl_loaded"), 0);
+  Boot(m);
   EXPECT_EQ(TestProperty(), "memtag");
   EXPECT_EQ(TestFlag(), "1");
 }
 
-TEST_F(MteCtrlTest, set_read_memtag) {
-  ASSERT_EQ(mtectrl("-s arm64.memtag.test_bootctl memtag"), 0);
-  EXPECT_EQ(TestProperty(), "memtag");
+TEST_F(MteCtrlTest, set_read_force_off) {
+  Boot({});
+  SetMemtagProp("memtag,memtag-once");
+  SetOverrideProp("force_off");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag-off,forced");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "none");
 }
 
-TEST_F(MteCtrlTest, set_read_force_off) {
-  ASSERT_EQ(mtectrl("-s arm64.memtag.test_bootctl memtag,memtag-once force_off"), 0);
-  EXPECT_EQ(TestProperty(), "memtag-once,memtag-off");
+TEST_F(MteCtrlTest, set_read_force_off_already) {
+  Boot({});
+  SetMemtagProp("memtag-off,memtag-once");
+  SetOverrideProp("force_off");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag-off");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag-off");
+}
+
+TEST_F(MteCtrlTest, set_read_force_on) {
+  Boot({});
+  SetMemtagProp("memtag-once");
+  SetOverrideProp("force_on");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag,forced");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "none");
+}
+
+TEST_F(MteCtrlTest, set_read_force_on_already) {
+  Boot({});
+  SetMemtagProp("memtag,memtag-once");
+  SetOverrideProp("force_on");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag");
 }
 
 TEST_F(MteCtrlTest, override) {
-  ASSERT_EQ(mtectrl("memtag"), 0);
-  ASSERT_EQ(mtectrl("memtag-once"), 0);
+  Boot({});
+  SetMemtagProp(("memtag"));
+  SetMemtagProp(("memtag-once"));
   EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x02"));
 }
 
 TEST_F(MteCtrlTest, read_empty) {
-  ASSERT_EQ(mtectrl("-s arm64.memtag.test_bootctl -f arm64.memtag.test_bootctl_loaded"), 0);
+  Boot({});
   EXPECT_EQ(TestProperty(), "none");
   EXPECT_EQ(TestFlag(), "1");
 }
 
 TEST_F(MteCtrlTest, force_off_invalid_mode) {
-  mtectrl("-s arm64.memtag.test_bootctl memtag-invalid force_off");
-  EXPECT_EQ(TestProperty(), "memtag-off");
-  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x10"));
+  Boot({});
+  SetMemtagProp("memtag-invalid");
+  SetOverrideProp("force_off");
+  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x30"));
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag-off,forced");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "none");
 }
 
 TEST_F(MteCtrlTest, force_on_invalid_mode) {
-  mtectrl("-s arm64.memtag.test_bootctl memtag-invalid force_on");
-  EXPECT_EQ(TestProperty(), "memtag");
-  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x01"));
+  Boot({});
+  SetMemtagProp("memtag-invalid");
+  SetOverrideProp("force_on");
+  EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x21"));
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag,forced");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "none");
 }
 
 TEST_F(MteCtrlTest, mode_invalid_override) {
-  mtectrl("-s arm64.memtag.test_bootctl memtag force_invalid");
-  EXPECT_EQ(TestProperty(), "memtag");
+  Boot({});
+  SetMemtagProp("memtag");
+  SetOverrideProp("force_invalid");
   EXPECT_THAT(GetMisc(), StartsWith("\x01\x5a\xfe\xfe\x5a\x01"));
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag");
+  SetOverrideProp("default");
+  Reboot();
+  EXPECT_EQ(TestProperty(), "memtag");
 }