From b615f3beac9681f074f56234142931c66b6e8fbb Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Wed, 9 Nov 2022 05:48:45 +0000 Subject: [PATCH] Defer CE key fixations to checkpoint commit On the first boot after an upgrade, ensure that any Keystore key deletions triggered by fscrypt_set_user_key_protection() are deferred until the userdata filesystem checkpoint is committed, so that the system doesn't end up in a bad state if the checkpoint is rolled back. Test: see I77d30f9be57de7b7c4818680732331549ecb73c8 Bug: 232452368 Ignore-AOSP-First: depends on other changes in internal master Change-Id: I59b758bc13b7a2ae270f1a6c409affe2eb61119c --- Checkpoint.cpp | 15 +++++++++++++++ FsCrypt.cpp | 32 +++++++++++++++++++++++++++++++- FsCrypt.h | 1 + KeyStorage.cpp | 10 ++++------ KeyStorage.h | 2 ++ 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/Checkpoint.cpp b/Checkpoint.cpp index 948231d..3825af9 100644 --- a/Checkpoint.cpp +++ b/Checkpoint.cpp @@ -16,6 +16,8 @@ #define LOG_TAG "Checkpoint" #include "Checkpoint.h" +#include "FsCrypt.h" +#include "KeyStorage.h" #include "VoldUtil.h" #include "VolumeManager.h" @@ -78,6 +80,18 @@ bool setBowState(std::string const& block_device, std::string const& state) { return true; } +// Do any work that was deferred until the userdata filesystem checkpoint was +// committed. This work involves the deletion of resources that aren't covered +// by the userdata filesystem checkpoint, e.g. Keystore keys. +void DoCheckpointCommittedWork() { + // Take the crypt lock to provide synchronization with the Binder calls that + // operate on key directories. + std::lock_guard lock(VolumeManager::Instance()->getCryptLock()); + + DeferredCommitKeystoreKeys(); + fscrypt_deferred_fixate_ce_keys(); +} + } // namespace Status cp_supportsCheckpoint(bool& result) { @@ -205,6 +219,7 @@ Status cp_commitChanges() { if (!android::base::RemoveFileIfExists(kMetadataCPFile, &err_str)) return error(err_str.c_str()); + std::thread(DoCheckpointCommittedWork).detach(); return Status::ok(); } diff --git a/FsCrypt.cpp b/FsCrypt.cpp index 477db7c..8df438f 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -16,6 +16,7 @@ #include "FsCrypt.h" +#include "Checkpoint.h" #include "KeyStorage.h" #include "KeyUtil.h" #include "Utils.h" @@ -112,6 +113,9 @@ EncryptionPolicy s_device_policy; std::map s_de_policies; std::map s_ce_policies; +// CE key fixation operations that have been deferred to checkpoint commit +std::map s_deferred_fixations; + } // namespace // Returns KeyGeneration suitable for key as described in EncryptionOptions @@ -214,6 +218,7 @@ static bool read_and_fixate_user_ce_key(userid_t user_id, LOG(DEBUG) << "Trying user CE key " << ce_key_path; if (retrieveKey(ce_key_path, auth, ce_key)) { LOG(DEBUG) << "Successfully retrieved key"; + s_deferred_fixations.erase(directory_path); fixate_user_ce_key(directory_path, ce_key_path, paths); return true; } @@ -676,6 +681,7 @@ bool fscrypt_destroy_user_key(userid_t user_id) { success &= android::vold::destroyKey(path); } } + s_deferred_fixations.erase(ce_path); success &= destroy_dir(ce_path); auto de_key_path = get_de_key_path(user_id); @@ -815,13 +821,37 @@ bool fscrypt_set_user_key_protection(userid_t user_id, const std::string& secret std::string ce_key_path; if (!get_ce_key_new_path(directory_path, paths, &ce_key_path)) return false; if (!android::vold::storeKeyAtomically(ce_key_path, user_key_temp, *auth, ce_key)) return false; - if (!fixate_user_ce_key(directory_path, ce_key_path, paths)) return false; + + // Fixate the key, i.e. delete all other bindings of it. (In practice this + // just means the kEmptyAuthentication binding, if there is one.) However, + // if a userdata filesystem checkpoint is pending, then we need to delay the + // fixation until the checkpoint has been committed, since deleting keys + // from Keystore cannot be rolled back. + if (android::vold::cp_needsCheckpoint()) { + LOG(INFO) << "Deferring fixation of " << directory_path << " until checkpoint is committed"; + s_deferred_fixations[directory_path] = ce_key_path; + } else { + s_deferred_fixations.erase(directory_path); + if (!fixate_user_ce_key(directory_path, ce_key_path, paths)) return false; + } + if (s_new_ce_keys.erase(user_id)) { LOG(INFO) << "Stored CE key for new user " << user_id; } return true; } +void fscrypt_deferred_fixate_ce_keys() { + for (const auto& it : s_deferred_fixations) { + const auto& directory_path = it.first; + const auto& to_fix = it.second; + LOG(INFO) << "Doing deferred fixation of " << directory_path; + fixate_user_ce_key(directory_path, to_fix, get_ce_key_paths(directory_path)); + // Continue on error. + } + s_deferred_fixations.clear(); +} + std::vector fscrypt_get_unlocked_users() { std::vector user_ids; for (const auto& it : s_ce_policies) { diff --git a/FsCrypt.h b/FsCrypt.h index 2cb47eb..8d84bdc 100644 --- a/FsCrypt.h +++ b/FsCrypt.h @@ -26,6 +26,7 @@ extern bool fscrypt_init_user0_done; bool fscrypt_vold_create_user_key(userid_t user_id, int serial, bool ephemeral); bool fscrypt_destroy_user_key(userid_t user_id); bool fscrypt_set_user_key_protection(userid_t user_id, const std::string& secret); +void fscrypt_deferred_fixate_ce_keys(); std::vector fscrypt_get_unlocked_users(); bool fscrypt_unlock_user_key(userid_t user_id, int serial, const std::string& secret); diff --git a/KeyStorage.cpp b/KeyStorage.cpp index eb994e9..33d415e 100644 --- a/KeyStorage.cpp +++ b/KeyStorage.cpp @@ -23,7 +23,6 @@ #include #include #include -#include #include #include @@ -231,9 +230,8 @@ static bool CommitUpgradedKey(Keystore& keystore, const std::string& dir) { return true; } -static void DeferredCommitKeys() { - android::base::WaitForProperty("vold.checkpoint_committed", "1"); - LOG(INFO) << "Committing upgraded keys"; +void DeferredCommitKeystoreKeys() { + LOG(INFO) << "Committing upgraded Keystore keys"; Keystore keystore; if (!keystore) { LOG(ERROR) << "Failed to open Keystore; old keys won't be deleted from Keystore"; @@ -241,10 +239,11 @@ static void DeferredCommitKeys() { } std::lock_guard lock(key_upgrade_lock); for (auto& dir : key_dirs_to_commit) { - LOG(INFO) << "Committing upgraded key " << dir; + LOG(INFO) << "Committing upgraded Keystore key for " << dir; CommitUpgradedKey(keystore, dir); } key_dirs_to_commit.clear(); + LOG(INFO) << "Done committing upgraded Keystore keys"; } // Returns true if the Keystore key in |dir| has already been upgraded and is @@ -260,7 +259,6 @@ static bool IsKeyCommitPending(const std::string& dir) { // that key_upgrade_lock is held and that a commit isn't already pending for the // directory. static void ScheduleKeyCommit(const std::string& dir) { - if (key_dirs_to_commit.empty()) std::thread(DeferredCommitKeys).detach(); key_dirs_to_commit.push_back(dir); } diff --git a/KeyStorage.h b/KeyStorage.h index cc2f549..22453ea 100644 --- a/KeyStorage.h +++ b/KeyStorage.h @@ -41,6 +41,8 @@ extern const KeyAuthentication kEmptyAuthentication; bool createSecdiscardable(const std::string& path, std::string* hash); bool readSecdiscardable(const std::string& path, std::string* hash); +void DeferredCommitKeystoreKeys(); + // Renames a key directory while also managing deferred commits appropriately. // This method should be used whenever a key directory needs to be moved/renamed. bool RenameKeyDir(const std::string& old_name, const std::string& new_name);