KeyStorage: rework key upgrade handling

Remove the error-prone 'keepOld' parameter, and instead make begin()
(renamed to BeginKeymasterOp()) do all the key upgrade handling.

Don't handle /data and /metadata differently anymore.  Previously, when
a checkpoint is active, key blob files were replaced on /data
immediately; only the actual Keymaster key deletion was delayed until
checkpoint commit.  But it's easier to just delay the key blob file
replacement too, as we have to implement that for /metadata anyway.

Also be more vigilant about deleting any leftover upgraded keys.

Test: Tested on bramble using an OTA rvc-d1-release => master.  In OTA
      success case, verified via logcat that the keys were upgraded and
      then were committed after the boot succeeded.  In OTA failure
      case, verified that the device still boots -- i.e., the old keys
      weren't lost.  Verified that in either case, no
      keymaster_key_blob_upgraded files were left over.  Finally, also
      tried 'pm create-user' and 'pm remove-user' and verified via
      logcat that the Keymaster keys still get deleted.
Change-Id: Ic9c3e63e0bcae0c608fc79050ca4a1676b3852ee

8 files changed, 181 insertions, 147 deletions

diff --git a/FsCrypt.cpp b/FsCrypt.cpp
index ebb46406..ff8c1f47 100644
--- a/FsCrypt.cpp
+++ b/FsCrypt.cpp
@@ -200,7 +200,7 @@ static bool read_and_fixate_user_ce_key(userid_t user_id,
     auto const paths = get_ce_key_paths(directory_path);
     for (auto const ce_key_path : paths) {
         LOG(DEBUG) << "Trying user CE key " << ce_key_path;
-        if (retrieveKey(ce_key_path, auth, ce_key, false)) {
+        if (retrieveKey(ce_key_path, auth, ce_key)) {
             LOG(DEBUG) << "Successfully retrieved key";
             fixate_user_ce_key(directory_path, ce_key_path, paths);
             return true;
@@ -401,7 +401,7 @@ static bool load_all_de_keys() {
         userid_t user_id = std::stoi(entry->d_name);
         auto key_path = de_dir + "/" + entry->d_name;
         KeyBuffer de_key;
-        if (!retrieveKey(key_path, kEmptyAuthentication, &de_key, false)) return false;
+        if (!retrieveKey(key_path, kEmptyAuthentication, &de_key)) return false;
         EncryptionPolicy de_policy;
         if (!install_storage_key(DATA_MNT_POINT, options, de_key, &de_policy)) return false;
         auto ret = s_de_policies.insert({user_id, de_policy});
@@ -435,7 +435,7 @@ bool fscrypt_initialize_systemwide_keys() {
 
     KeyBuffer device_key;
     if (!retrieveOrGenerateKey(device_key_path, device_key_temp, kEmptyAuthentication,
-                               makeGen(options), &device_key, false))
+                               makeGen(options), &device_key))
         return false;
 
     EncryptionPolicy device_policy;
@@ -669,7 +669,7 @@ static bool read_or_create_volkey(const std::string& misc_path, const std::strin
     EncryptionOptions options;
     if (!get_volume_file_encryption_options(&options)) return false;
     KeyBuffer key;
-    if (!retrieveOrGenerateKey(key_path, key_path + "_tmp", auth, makeGen(options), &key, false))
+    if (!retrieveOrGenerateKey(key_path, key_path + "_tmp", auth, makeGen(options), &key))
         return false;
     if (!install_storage_key(BuildDataPath(volume_uuid), options, key, policy)) return false;
     return true;
@@ -688,12 +688,12 @@ static bool fscrypt_rewrap_user_key(userid_t user_id, int serial,
     auto const directory_path = get_ce_key_directory_path(user_id);
     KeyBuffer ce_key;
     std::string ce_key_current_path = get_ce_key_current_path(directory_path);
-    if (retrieveKey(ce_key_current_path, retrieve_auth, &ce_key, false)) {
+    if (retrieveKey(ce_key_current_path, retrieve_auth, &ce_key)) {
         LOG(DEBUG) << "Successfully retrieved key";
         // TODO(147732812): Remove this once Locksettingservice is fixed.
         // Currently it calls fscrypt_clear_user_key_auth with a secret when lockscreen is
         // changed from swipe to none or vice-versa
-    } else if (retrieveKey(ce_key_current_path, kEmptyAuthentication, &ce_key, false)) {
+    } else if (retrieveKey(ce_key_current_path, kEmptyAuthentication, &ce_key)) {
         LOG(DEBUG) << "Successfully retrieved key with empty auth";
     } else {
         LOG(ERROR) << "Failed to retrieve key for user " << user_id;
diff --git a/KeyStorage.cpp b/KeyStorage.cpp
index 533a7cbf..81478277 100644
--- a/KeyStorage.cpp
+++ b/KeyStorage.cpp
@@ -21,6 +21,7 @@
 #include "ScryptParameters.h"
 #include "Utils.h"
 
+#include <algorithm>
 #include <thread>
 #include <vector>
 
@@ -208,75 +209,161 @@ bool readSecdiscardable(const std::string& filename, std::string* hash) {
     return true;
 }
 
-static void deferedKmDeleteKey(const std::string& kmkey) {
-    while (!android::base::WaitForProperty("vold.checkpoint_committed", "1")) {
-        LOG(ERROR) << "Wait for boot timed out";
+static std::mutex key_upgrade_lock;
+
+// List of key directories that have had their Keymaster key upgraded during
+// this boot and written to "keymaster_key_blob_upgraded", but replacing the old
+// key was delayed due to an active checkpoint.  Protected by key_upgrade_lock.
+static std::vector<std::string> key_dirs_to_commit;
+
+// Replaces |dir|/keymaster_key_blob with |dir|/keymaster_key_blob_upgraded and
+// deletes the old key from Keymaster.
+static bool CommitUpgradedKey(Keymaster& keymaster, const std::string& dir) {
+    auto blob_file = dir + "/" + kFn_keymaster_key_blob;
+    auto upgraded_blob_file = dir + "/" + kFn_keymaster_key_blob_upgraded;
+
+    std::string blob;
+    if (!readFileToString(blob_file, &blob)) return false;
+
+    if (rename(upgraded_blob_file.c_str(), blob_file.c_str()) != 0) {
+        PLOG(ERROR) << "Failed to rename " << upgraded_blob_file << " to " << blob_file;
+        return false;
     }
+    // Ensure that the rename is persisted before deleting the Keymaster key.
+    if (!FsyncDirectory(dir)) return false;
+
+    if (!keymaster || !keymaster.deleteKey(blob)) {
+        LOG(WARNING) << "Failed to delete old key " << blob_file
+                     << " from Keymaster; continuing anyway";
+        // Continue on, but the space in Keymaster used by the old key won't be freed.
+    }
+    return true;
+}
+
+static void DeferredCommitKeys() {
+    android::base::WaitForProperty("vold.checkpoint_committed", "1");
+    LOG(INFO) << "Committing upgraded keys";
     Keymaster keymaster;
-    if (!keymaster || !keymaster.deleteKey(kmkey)) {
-        LOG(ERROR) << "Defered Key deletion failed during upgrade";
+    if (!keymaster) {
+        LOG(ERROR) << "Failed to open Keymaster; old keys won't be deleted from Keymaster";
+        // Continue on, but the space in Keymaster used by the old keys won't be freed.
+    }
+    std::lock_guard<std::mutex> lock(key_upgrade_lock);
+    for (auto& dir : key_dirs_to_commit) {
+        LOG(INFO) << "Committing upgraded key " << dir;
+        CommitUpgradedKey(keymaster, dir);
     }
+    key_dirs_to_commit.clear();
 }
 
-bool kmDeleteKey(Keymaster& keymaster, const std::string& kmKey) {
-    bool needs_cp = cp_needsCheckpoint();
+// Returns true if the Keymaster key in |dir| has already been upgraded and is
+// pending being committed.  Assumes that key_upgrade_lock is held.
+static bool IsKeyCommitPending(const std::string& dir) {
+    for (const auto& dir_to_commit : key_dirs_to_commit) {
+        if (IsSameFile(dir, dir_to_commit)) return true;
+    }
+    return false;
+}
 
-    if (needs_cp) {
-        std::thread(deferedKmDeleteKey, kmKey).detach();
-        LOG(INFO) << "Deferring Key deletion during upgrade";
-        return true;
-    } else {
-        return keymaster.deleteKey(kmKey);
+// Schedules the upgraded Keymaster key in |dir| to be committed later.
+// Assumes that key_upgrade_lock is held.
+static void ScheduleKeyCommit(const std::string& dir) {
+    if (key_dirs_to_commit.empty()) std::thread(DeferredCommitKeys).detach();
+    key_dirs_to_commit.push_back(dir);
+}
+
+static void CancelPendingKeyCommit(const std::string& dir) {
+    std::lock_guard<std::mutex> lock(key_upgrade_lock);
+    for (auto it = key_dirs_to_commit.begin(); it != key_dirs_to_commit.end(); it++) {
+        if (IsSameFile(*it, dir)) {
+            LOG(DEBUG) << "Cancelling pending commit of upgraded key " << dir
+                       << " because it is being destroyed";
+            key_dirs_to_commit.erase(it);
+            break;
+        }
     }
 }
 
-static KeymasterOperation begin(Keymaster& keymaster, const std::string& dir,
-                                km::KeyPurpose purpose, const km::AuthorizationSet& keyParams,
-                                const km::AuthorizationSet& opParams,
-                                const km::HardwareAuthToken& authToken,
-                                km::AuthorizationSet* outParams, bool keepOld) {
-    auto kmKeyPath = dir + "/" + kFn_keymaster_key_blob;
-    std::string kmKey;
-    if (!readFileToString(kmKeyPath, &kmKey)) return KeymasterOperation();
-    km::AuthorizationSet inParams(keyParams);
-    inParams.append(opParams.begin(), opParams.end());
-    for (;;) {
-        auto opHandle = keymaster.begin(purpose, kmKey, inParams, authToken, outParams);
-        if (opHandle) {
-            return opHandle;
+// Deletes a leftover upgraded key, if present.  An upgraded key can be left
+// over if an update failed, or if we rebooted before committing the key in a
+// freak accident.  Either way, we can re-upgrade the key if we need to.
+static void DeleteUpgradedKey(Keymaster& keymaster, const std::string& path) {
+    if (pathExists(path)) {
+        LOG(DEBUG) << "Deleting leftover upgraded key " << path;
+        std::string blob;
+        if (!android::base::ReadFileToString(path, &blob)) {
+            LOG(WARNING) << "Failed to read leftover upgraded key " << path
+                         << "; continuing anyway";
+        } else if (!keymaster.deleteKey(blob)) {
+            LOG(WARNING) << "Failed to delete leftover upgraded key " << path
+                         << " from Keymaster; continuing anyway";
         }
-        if (opHandle.errorCode() != km::ErrorCode::KEY_REQUIRES_UPGRADE) return opHandle;
-        LOG(DEBUG) << "Upgrading key: " << dir;
-        std::string newKey;
-        if (!keymaster.upgradeKey(kmKey, keyParams, &newKey)) return KeymasterOperation();
-        auto newKeyPath = dir + "/" + kFn_keymaster_key_blob_upgraded;
-        if (!writeStringToFile(newKey, newKeyPath)) return KeymasterOperation();
-        if (!keepOld) {
-            if (rename(newKeyPath.c_str(), kmKeyPath.c_str()) != 0) {
-                PLOG(ERROR) << "Unable to move upgraded key to location: " << kmKeyPath;
-                return KeymasterOperation();
-            }
-            if (!android::vold::FsyncDirectory(dir)) {
-                LOG(ERROR) << "Key dir sync failed: " << dir;
-                return KeymasterOperation();
-            }
-            if (!kmDeleteKey(keymaster, kmKey)) {
-                LOG(ERROR) << "Key deletion failed during upgrade, continuing anyway: " << dir;
-            }
+        if (unlink(path.c_str()) != 0) {
+            LOG(WARNING) << "Failed to unlink leftover upgraded key " << path
+                         << "; continuing anyway";
         }
-        kmKey = newKey;
-        LOG(INFO) << "Key upgraded: " << dir;
     }
 }
 
+// Begins a Keymaster operation using the key stored in |dir|.
+static KeymasterOperation BeginKeymasterOp(Keymaster& keymaster, const std::string& dir,
+                                           km::KeyPurpose purpose,
+                                           const km::AuthorizationSet& keyParams,
+                                           const km::AuthorizationSet& opParams,
+                                           const km::HardwareAuthToken& authToken,
+                                           km::AuthorizationSet* outParams) {
+    km::AuthorizationSet inParams(keyParams);
+    inParams.append(opParams.begin(), opParams.end());
+
+    auto blob_file = dir + "/" + kFn_keymaster_key_blob;
+    auto upgraded_blob_file = dir + "/" + kFn_keymaster_key_blob_upgraded;
+
+    std::lock_guard<std::mutex> lock(key_upgrade_lock);
+
+    std::string blob;
+    bool already_upgraded = IsKeyCommitPending(dir);
+    if (already_upgraded) {
+        LOG(DEBUG)
+                << blob_file
+                << " was already upgraded and is waiting to be committed; using the upgraded blob";
+        if (!readFileToString(upgraded_blob_file, &blob)) return KeymasterOperation();
+    } else {
+        DeleteUpgradedKey(keymaster, upgraded_blob_file);
+        if (!readFileToString(blob_file, &blob)) return KeymasterOperation();
+    }
+
+    auto opHandle = keymaster.begin(purpose, blob, inParams, authToken, outParams);
+    if (opHandle) return opHandle;
+    if (opHandle.errorCode() != km::ErrorCode::KEY_REQUIRES_UPGRADE) return opHandle;
+
+    if (already_upgraded) {
+        LOG(ERROR) << "Unexpected case; already-upgraded key " << upgraded_blob_file
+                   << " still requires upgrade";
+        return KeymasterOperation();
+    }
+    LOG(INFO) << "Upgrading key: " << blob_file;
+    if (!keymaster.upgradeKey(blob, keyParams, &blob)) return KeymasterOperation();
+    if (!writeStringToFile(blob, upgraded_blob_file)) return KeymasterOperation();
+    if (cp_needsCheckpoint()) {
+        LOG(INFO) << "Wrote upgraded key to " << upgraded_blob_file
+                  << "; delaying commit due to checkpoint";
+        ScheduleKeyCommit(dir);
+    } else {
+        if (!CommitUpgradedKey(keymaster, dir)) return KeymasterOperation();
+        LOG(INFO) << "Key upgraded: " << blob_file;
+    }
+
+    return keymaster.begin(purpose, blob, inParams, authToken, outParams);
+}
+
 static bool encryptWithKeymasterKey(Keymaster& keymaster, const std::string& dir,
                                     const km::AuthorizationSet& keyParams,
-                                    const km::HardwareAuthToken& authToken, const KeyBuffer& message,
-                                    std::string* ciphertext, bool keepOld) {
+                                    const km::HardwareAuthToken& authToken,
+                                    const KeyBuffer& message, std::string* ciphertext) {
     km::AuthorizationSet opParams;
     km::AuthorizationSet outParams;
-    auto opHandle = begin(keymaster, dir, km::KeyPurpose::ENCRYPT, keyParams, opParams, authToken,
-                          &outParams, keepOld);
+    auto opHandle = BeginKeymasterOp(keymaster, dir, km::KeyPurpose::ENCRYPT, keyParams, opParams,
+                                     authToken, &outParams);
     if (!opHandle) return false;
     auto nonceBlob = outParams.GetTagValue(km::TAG_NONCE);
     if (!nonceBlob.isOk()) {
@@ -300,14 +387,13 @@ static bool encryptWithKeymasterKey(Keymaster& keymaster, const std::string& dir
 static bool decryptWithKeymasterKey(Keymaster& keymaster, const std::string& dir,
                                     const km::AuthorizationSet& keyParams,
                                     const km::HardwareAuthToken& authToken,
-                                    const std::string& ciphertext, KeyBuffer* message,
-                                    bool keepOld) {
+                                    const std::string& ciphertext, KeyBuffer* message) {
     auto nonce = ciphertext.substr(0, GCM_NONCE_BYTES);
     auto bodyAndMac = ciphertext.substr(GCM_NONCE_BYTES);
     auto opParams = km::AuthorizationSetBuilder().Authorization(km::TAG_NONCE,
                                                                 km::support::blob2hidlVec(nonce));
-    auto opHandle = begin(keymaster, dir, km::KeyPurpose::DECRYPT, keyParams, opParams, authToken,
-                          nullptr, keepOld);
+    auto opHandle = BeginKeymasterOp(keymaster, dir, km::KeyPurpose::DECRYPT, keyParams, opParams,
+                                     authToken, nullptr);
     if (!opHandle) return false;
     if (!opHandle.updateCompletely(bodyAndMac, message)) return false;
     if (!opHandle.finish(nullptr)) return false;
@@ -513,8 +599,7 @@ bool storeKey(const std::string& dir, const KeyAuthentication& auth, const KeyBu
         km::AuthorizationSet keyParams;
         km::HardwareAuthToken authToken;
         std::tie(keyParams, authToken) = beginParams(auth, appId);
-        if (!encryptWithKeymasterKey(keymaster, dir, keyParams, authToken, key, &encryptedKey,
-                                     false))
+        if (!encryptWithKeymasterKey(keymaster, dir, keyParams, authToken, key, &encryptedKey))
             return false;
     } else {
         if (!encryptWithoutKeymaster(appId, key, &encryptedKey)) return false;
@@ -543,8 +628,7 @@ bool storeKeyAtomically(const std::string& key_path, const std::string& tmp_path
     return true;
 }
 
-bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffer* key,
-                 bool keepOld) {
+bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffer* key) {
     std::string version;
     if (!readFileToString(dir + "/" + kFn_version, &version)) return false;
     if (version != kCurrentVersion) {
@@ -569,8 +653,7 @@ bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffe
         km::AuthorizationSet keyParams;
         km::HardwareAuthToken authToken;
         std::tie(keyParams, authToken) = beginParams(auth, appId);
-        if (!decryptWithKeymasterKey(keymaster, dir, keyParams, authToken, encryptedMessage, key,
-                                     keepOld))
+        if (!decryptWithKeymasterKey(keymaster, dir, keyParams, authToken, encryptedMessage, key))
             return false;
     } else {
         if (!decryptWithoutKeymaster(appId, encryptedMessage, key)) return false;
@@ -578,12 +661,13 @@ bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffe
     return true;
 }
 
-static bool deleteKey(const std::string& dir) {
-    std::string kmKey;
-    if (!readFileToString(dir + "/" + kFn_keymaster_key_blob, &kmKey)) return false;
+static bool DeleteKeymasterKey(const std::string& blob_file) {
+    std::string blob;
+    if (!readFileToString(blob_file, &blob)) return false;
     Keymaster keymaster;
     if (!keymaster) return false;
-    if (!keymaster.deleteKey(kmKey)) return false;
+    LOG(DEBUG) << "Deleting key " << blob_file << " from Keymaster";
+    if (!keymaster.deleteKey(blob)) return false;
     return true;
 }
 
@@ -605,19 +689,23 @@ static bool recursiveDeleteKey(const std::string& dir) {
 
 bool destroyKey(const std::string& dir) {
     bool success = true;
-    // Try each thing, even if previous things failed.
-    bool uses_km = pathExists(dir + "/" + kFn_keymaster_key_blob);
-    if (uses_km) {
-        success &= deleteKey(dir);
-    }
+
+    CancelPendingKeyCommit(dir);
+
     auto secdiscard_cmd = std::vector<std::string>{
         kSecdiscardPath,
         "--",
         dir + "/" + kFn_encrypted_key,
         dir + "/" + kFn_secdiscardable,
     };
-    if (uses_km) {
-        secdiscard_cmd.emplace_back(dir + "/" + kFn_keymaster_key_blob);
+    // Try each thing, even if previous things failed.
+
+    for (auto& fn : {kFn_keymaster_key_blob, kFn_keymaster_key_blob_upgraded}) {
+        auto blob_file = dir + "/" + fn;
+        if (pathExists(blob_file)) {
+            success &= DeleteKeymasterKey(blob_file);
+            secdiscard_cmd.push_back(blob_file);
+        }
     }
     if (ForkExecvp(secdiscard_cmd) != 0) {
         LOG(ERROR) << "secdiscard failed";
diff --git a/KeyStorage.h b/KeyStorage.h
index 5228f084..1eb26ae1 100644
--- a/KeyStorage.h
+++ b/KeyStorage.h
@@ -61,20 +61,7 @@ bool storeKeyAtomically(const std::string& key_path, const std::string& tmp_path
                         const KeyAuthentication& auth, const KeyBuffer& key);
 
 // Retrieve the key from the named directory.
-//
-// If the key is wrapped by a Keymaster key that requires an upgrade, then that
-// Keymaster key is upgraded.  If |keepOld| is false, then the upgraded
-// Keymaster key replaces the original one.  As part of this, the original is
-// deleted from Keymaster; however, if a user data checkpoint is active, this
-// part is delayed until the checkpoint is committed.
-//
-// If instead |keepOld| is true, then the upgraded key doesn't actually replace
-// the original one.  This is needed *only* if |dir| isn't located in /data and
-// a user data checkpoint is active.  In this case the caller must handle
-// replacing the original key if the checkpoint is committed, and deleting the
-// upgraded key if the checkpoint is rolled back.
-bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffer* key,
-                 bool keepOld);
+bool retrieveKey(const std::string& dir, const KeyAuthentication& auth, KeyBuffer* key);
 
 // Securely destroy the key stored in the named directory and delete the directory.
 bool destroyKey(const std::string& dir);
diff --git a/KeyUtil.cpp b/KeyUtil.cpp
index f3a29869..9d01e1ea 100644
--- a/KeyUtil.cpp
+++ b/KeyUtil.cpp
@@ -392,10 +392,10 @@ bool evictKey(const std::string& mountpoint, const EncryptionPolicy& policy) {
 
 bool retrieveOrGenerateKey(const std::string& key_path, const std::string& tmp_path,
                            const KeyAuthentication& key_authentication, const KeyGeneration& gen,
-                           KeyBuffer* key, bool keepOld) {
+                           KeyBuffer* key) {
     if (pathExists(key_path)) {
         LOG(DEBUG) << "Key exists, using: " << key_path;
-        if (!retrieveKey(key_path, key_authentication, key, keepOld)) return false;
+        if (!retrieveKey(key_path, key_authentication, key)) return false;
     } else {
         if (!gen.allow_gen) {
             LOG(ERROR) << "No key found in " << key_path;
diff --git a/KeyUtil.h b/KeyUtil.h
index 0f5bc93e..73255a30 100644
--- a/KeyUtil.h
+++ b/KeyUtil.h
@@ -75,10 +75,10 @@ bool installKey(const std::string& mountpoint, const EncryptionOptions& options,
 bool evictKey(const std::string& mountpoint, const EncryptionPolicy& policy);
 
 // Retrieves the key from the named directory, or generates it if it doesn't
-// exist.  In most cases |keepOld| must be false; see retrieveKey() for details.
+// exist.
 bool retrieveOrGenerateKey(const std::string& key_path, const std::string& tmp_path,
                            const KeyAuthentication& key_authentication, const KeyGeneration& gen,
-                           KeyBuffer* key, bool keepOld);
+                           KeyBuffer* key);
 
 // Re-installs a file-based encryption key of fscrypt-provisioning type from the
 // global session keyring back into fs keyring of the mountpoint.
diff --git a/MetadataCrypt.cpp b/MetadataCrypt.cpp
index f794ee33..fdee21fa 100644
--- a/MetadataCrypt.cpp
+++ b/MetadataCrypt.cpp
@@ -17,17 +17,13 @@
 #include "MetadataCrypt.h"
 #include "KeyBuffer.h"
 
-#include <algorithm>
 #include <string>
-#include <thread>
-#include <vector>
 
 #include <fcntl.h>
 #include <sys/param.h>
 #include <sys/stat.h>
 #include <sys/types.h>
 
-#include <android-base/file.h>
 #include <android-base/logging.h>
 #include <android-base/properties.h>
 #include <android-base/strings.h>
@@ -64,9 +60,6 @@ struct CryptoOptions {
 
 static const std::string kDmNameUserdata = "userdata";
 
-static const char* kFn_keymaster_key_blob = "keymaster_key_blob";
-static const char* kFn_keymaster_key_blob_upgraded = "keymaster_key_blob_upgraded";
-
 // The first entry in this table is the default crypto type.
 constexpr CryptoType supported_crypto_types[] = {aes_256_xts, adiantum};
 
@@ -111,31 +104,6 @@ static bool mount_via_fs_mgr(const char* mount_point, const char* blk_device) {
     return true;
 }
 
-// Note: It is possible to orphan a key if it is removed before deleting
-// Update this once keymaster APIs change, and we have a proper commit.
-static void commit_key(const std::string& dir) {
-    while (!android::base::WaitForProperty("vold.checkpoint_committed", "1")) {
-        LOG(ERROR) << "Wait for boot timed out";
-    }
-    Keymaster keymaster;
-    auto keyPath = dir + "/" + kFn_keymaster_key_blob;
-    auto newKeyPath = dir + "/" + kFn_keymaster_key_blob_upgraded;
-    std::string key;
-
-    if (!android::base::ReadFileToString(keyPath, &key)) {
-        LOG(ERROR) << "Failed to read old key: " << dir;
-        return;
-    }
-    if (rename(newKeyPath.c_str(), keyPath.c_str()) != 0) {
-        PLOG(ERROR) << "Unable to move upgraded key to location: " << keyPath;
-        return;
-    }
-    if (!keymaster.deleteKey(key)) {
-        LOG(ERROR) << "Key deletion failed during upgrade, continuing anyway: " << dir;
-    }
-    LOG(INFO) << "Old Key deleted: " << dir;
-}
-
 static bool read_key(const std::string& metadata_key_dir, const KeyGeneration& gen,
                      KeyBuffer* key) {
     if (metadata_key_dir.empty()) {
@@ -150,25 +118,7 @@ static bool read_key(const std::string& metadata_key_dir, const KeyGeneration& g
         return false;
     }
     auto temp = metadata_key_dir + "/tmp";
-    auto newKeyPath = dir + "/" + kFn_keymaster_key_blob_upgraded;
-    /* If we have a leftover upgraded key, delete it.
-     * We either failed an update and must return to the old key,
-     * or we rebooted before commiting the keys in a freak accident.
-     * Either way, we can re-upgrade the key if we need to.
-     */
-    Keymaster keymaster;
-    if (pathExists(newKeyPath)) {
-        if (!android::base::ReadFileToString(newKeyPath, &sKey))
-            LOG(ERROR) << "Failed to read incomplete key: " << dir;
-        else if (!keymaster.deleteKey(sKey))
-            LOG(ERROR) << "Incomplete key deletion failed, continuing anyway: " << dir;
-        else
-            unlink(newKeyPath.c_str());
-    }
-    bool needs_cp = cp_needsCheckpoint();
-    if (!retrieveOrGenerateKey(dir, temp, kEmptyAuthentication, gen, key, needs_cp)) return false;
-    if (needs_cp && pathExists(newKeyPath)) std::thread(commit_key, dir).detach();
-    return true;
+    return retrieveOrGenerateKey(dir, temp, kEmptyAuthentication, gen, key);
 }
 
 static bool get_number_of_sectors(const std::string& real_blkdev, uint64_t* nr_sec) {
diff --git a/Utils.cpp b/Utils.cpp
index 7f53a925..afb0989e 100644
--- a/Utils.cpp
+++ b/Utils.cpp
@@ -1129,6 +1129,13 @@ dev_t GetDevice(const std::string& path) {
     }
 }
 
+// Returns true if |path1| names the same existing file or directory as |path2|.
+bool IsSameFile(const std::string& path1, const std::string& path2) {
+    struct stat stbuf1, stbuf2;
+    if (stat(path1.c_str(), &stbuf1) != 0 || stat(path2.c_str(), &stbuf2) != 0) return false;
+    return stbuf1.st_ino == stbuf2.st_ino && stbuf1.st_dev == stbuf2.st_dev;
+}
+
 status_t RestoreconRecursive(const std::string& path) {
     LOG(DEBUG) << "Starting restorecon of " << path;
 
diff --git a/Utils.h b/Utils.h
index 27889c6b..cf3fd9b8 100644
--- a/Utils.h
+++ b/Utils.h
@@ -155,6 +155,8 @@ std::string BuildDataUserDePath(const std::string& volumeUuid, userid_t userid);
 
 dev_t GetDevice(const std::string& path);
 
+bool IsSameFile(const std::string& path1, const std::string& path2);
+
 status_t EnsureDirExists(const std::string& path, mode_t mode, uid_t uid, gid_t gid);
 
 status_t RestoreconRecursive(const std::string& path);