From 0f890a93e191777f885590384c33139646ea26b8 Mon Sep 17 00:00:00 2001
From: Satya Tangirala <satyat@google.com>
Date: Tue, 8 Jun 2021 12:55:24 -0700
Subject: [PATCH] Always use RenameKeyDir() when moving/renaming key
 directories

Make fixate_user_ce_key() use RenameKeyDir() to rename key directories
so that any deferred commits for these directories are also updated
appropriately.

This fixes a potential lost Keymaster key upgrade if a key were to be
re-wrapped while a user data checkpoint is pending.  This isn't a huge
issue as the key will just get upgraded again, but this should be fixed.

[ebiggers@ - cleaned up slightly from satyat@'s original change]

Bug: 190398249
Change-Id: Ic6c5b4468d07ab335368e3d373916145d096af01
---
 FsCrypt.cpp    |  5 +----
 KeyStorage.cpp | 16 ++++++++--------
 KeyStorage.h   |  4 ++++
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/FsCrypt.cpp b/FsCrypt.cpp
index 765073d..017ffec 100644
--- a/FsCrypt.cpp
+++ b/FsCrypt.cpp
@@ -186,10 +186,7 @@ static void fixate_user_ce_key(const std::string& directory_path, const std::str
     auto const current_path = get_ce_key_current_path(directory_path);
     if (to_fix != current_path) {
         LOG(DEBUG) << "Renaming " << to_fix << " to " << current_path;
-        if (rename(to_fix.c_str(), current_path.c_str()) != 0) {
-            PLOG(WARNING) << "Unable to rename " << to_fix << " to " << current_path;
-            return;
-        }
+        if (!android::vold::RenameKeyDir(to_fix, current_path)) return;
     }
     android::vold::FsyncDirectory(directory_path);
 }
diff --git a/KeyStorage.cpp b/KeyStorage.cpp
index af624e3..4893c2f 100644
--- a/KeyStorage.cpp
+++ b/KeyStorage.cpp
@@ -288,9 +288,7 @@ static void CancelPendingKeyCommit(const std::string& dir) {
     }
 }
 
-// Renames a key directory. Also updates the deferred commit vector
-// (key_dirs_to_commit) appropriately.
-static bool RenameKeyDir(const std::string& old_name, const std::string& new_name) {
+bool RenameKeyDir(const std::string& old_name, const std::string& new_name) {
     std::lock_guard<std::mutex> lock(key_upgrade_lock);
 
     // Find the entry in key_dirs_to_commit (if any) for this directory so that
@@ -301,7 +299,11 @@ static bool RenameKeyDir(const std::string& old_name, const std::string& new_nam
         if (IsSameFile(old_name, *it)) break;
     }
 
-    if (rename(old_name.c_str(), new_name.c_str()) != 0) return false;
+    if (rename(old_name.c_str(), new_name.c_str()) != 0) {
+        PLOG(ERROR) << "Failed to rename key directory \"" << old_name << "\" to \"" << new_name
+                    << "\"";
+        return false;
+    }
 
     if (it != key_dirs_to_commit.end()) *it = new_name;
 
@@ -614,10 +616,8 @@ bool storeKeyAtomically(const std::string& key_path, const std::string& tmp_path
     }
     if (!storeKey(tmp_path, auth, key)) return false;
 
-    if (!RenameKeyDir(tmp_path, key_path)) {
-        PLOG(ERROR) << "Unable to move new key to location: " << key_path;
-        return false;
-    }
+    if (!RenameKeyDir(tmp_path, key_path)) return false;
+
     if (!FsyncParentDirectory(key_path)) return false;
     LOG(DEBUG) << "Created key: " << key_path;
     return true;
diff --git a/KeyStorage.h b/KeyStorage.h
index e318959..de719e9 100644
--- a/KeyStorage.h
+++ b/KeyStorage.h
@@ -41,6 +41,10 @@ extern const KeyAuthentication kEmptyAuthentication;
 bool createSecdiscardable(const std::string& path, std::string* hash);
 bool readSecdiscardable(const std::string& path, std::string* hash);
 
+// 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);
+
 // Create a directory at the named path, and store "key" in it,
 // in such a way that it can only be retrieved via Keymaster and
 // can be securely deleted.