From a21962b207ebab74c333c95abaca103bc938f38d Mon Sep 17 00:00:00 2001 From: Nathan Huckleberry Date: Wed, 22 Feb 2023 02:28:28 +0000 Subject: [PATCH] Clean up potential busy files after key eviction. There is a race condition between key eviction and killing user processes. The race condition is difficult to properly fix without significantly degrading UI performance. If the race condition occurs, decrypted filesystem data is left in various kernel caches. To mitigate, we try to ensure the caches are flushed by evicting the keys again in a worker thread. Test: Checked that the correct log messages appear when evicting a user's keys Bug: 140762419 Change-Id: I9e39e5bb0f5190284552bcd252b6213a22a51e91 --- KeyUtil.cpp | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/KeyUtil.cpp b/KeyUtil.cpp index 25d5af3..395b6b3 100644 --- a/KeyUtil.cpp +++ b/KeyUtil.cpp @@ -19,6 +19,7 @@ #include #include #include +#include #include #include @@ -39,6 +40,10 @@ namespace vold { using android::fscrypt::EncryptionOptions; using android::fscrypt::EncryptionPolicy; +// This must be acquired before calling fscrypt ioctls that operate on keys. +// This prevents race conditions between evicting and reinstalling keys. +static std::mutex fscrypt_keyring_mutex; + const KeyGeneration neverGen() { return KeyGeneration{0, false, false}; } @@ -267,6 +272,7 @@ static bool installFsKeyringKey(const std::string& mountpoint, const EncryptionO bool installKey(const std::string& mountpoint, const EncryptionOptions& options, const KeyBuffer& key, EncryptionPolicy* policy) { + const std::lock_guard lock(fscrypt_keyring_mutex); policy->options = options; // Put the fscrypt_add_key_arg in an automatically-zeroing buffer, since we // have to copy the raw key into it. @@ -360,7 +366,66 @@ static bool evictProvisioningKey(const std::string& ref) { return true; } +static void waitForBusyFiles(const struct fscrypt_key_specifier key_spec, const std::string ref, + const std::string mountpoint) { + android::base::unique_fd fd(open(mountpoint.c_str(), O_RDONLY | O_DIRECTORY | O_CLOEXEC)); + if (fd == -1) { + PLOG(ERROR) << "Failed to open " << mountpoint << " to evict key"; + return; + } + + std::chrono::milliseconds wait_time(3200); + std::chrono::milliseconds total_wait_time(0); + while (wait_time <= std::chrono::milliseconds(51200)) { + total_wait_time += wait_time; + std::this_thread::sleep_for(wait_time); + + const std::lock_guard lock(fscrypt_keyring_mutex); + + struct fscrypt_get_key_status_arg get_arg; + memset(&get_arg, 0, sizeof(get_arg)); + get_arg.key_spec = key_spec; + + if (ioctl(fd, FS_IOC_GET_ENCRYPTION_KEY_STATUS, &get_arg) != 0) { + PLOG(ERROR) << "Failed to get status for fscrypt key with ref " << ref << " from " + << mountpoint; + return; + } + if (get_arg.status != FSCRYPT_KEY_STATUS_INCOMPLETELY_REMOVED) { + LOG(DEBUG) << "Key status changed, cancelling busy file cleanup for key with ref " + << ref << "."; + return; + } + + struct fscrypt_remove_key_arg remove_arg; + memset(&remove_arg, 0, sizeof(remove_arg)); + remove_arg.key_spec = key_spec; + + if (ioctl(fd, FS_IOC_REMOVE_ENCRYPTION_KEY, &remove_arg) != 0) { + PLOG(ERROR) << "Failed to clean up busy files for fscrypt key with ref " << ref + << " from " << mountpoint; + return; + } + if (remove_arg.removal_status_flags & FSCRYPT_KEY_REMOVAL_STATUS_FLAG_OTHER_USERS) { + // Should never happen because keys are only added/removed as root. + LOG(ERROR) << "Unexpected case: key with ref " << ref + << " is still added by other users!"; + } else if (!(remove_arg.removal_status_flags & + FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY)) { + LOG(INFO) << "Successfully cleaned up busy files for key with ref " << ref + << ". After waiting " << total_wait_time.count() << "ms."; + return; + } + LOG(WARNING) << "Files still open after waiting " << total_wait_time.count() + << "ms. Key with ref " << ref << " still has unlocked files!"; + wait_time *= 2; + } + LOG(ERROR) << "Waiting for files to close never completed. Files using key with ref " << ref + << " were not locked!"; +} + bool evictKey(const std::string& mountpoint, const EncryptionPolicy& policy) { + const std::lock_guard lock(fscrypt_keyring_mutex); if (policy.options.version == 1 && !isFsKeyringSupported()) { return evictKeyLegacy(policy.key_raw_ref); } @@ -390,8 +455,14 @@ bool evictKey(const std::string& mountpoint, const EncryptionPolicy& policy) { // Should never happen because keys are only added/removed as root. LOG(ERROR) << "Unexpected case: key with ref " << ref << " is still added by other users!"; } else if (arg.removal_status_flags & FSCRYPT_KEY_REMOVAL_STATUS_FLAG_FILES_BUSY) { - LOG(ERROR) << "Files still open after removing key with ref " << ref - << ". These files were not locked!"; + LOG(WARNING) + << "Files still open after removing key with ref " << ref + << ". These files were not locked! Punting busy file clean up to worker thread."; + // Processes are killed asynchronously in ActivityManagerService due to performance issues + // with synchronous kills. If there were busy files they will probably be killed soon. Wait + // for them asynchronously. + std::thread busyFilesThread(waitForBusyFiles, arg.key_spec, ref, mountpoint); + busyFilesThread.detach(); } if (!evictProvisioningKey(ref)) return false;