commit 0f890a93e191777f885590384c33139646ea26b8
author Satya Tangirala <satyat@google.com> Tue Jun 08 12:55:24 2021 -0700
committer Eric Biggers <ebiggers@google.com> Tue Jun 08 15:57:31 2021 -0700
tree 82135d195b8cffe9d5209343e1f8b6065b4ffda1
parent 107d21d4842ff8764decd4575d69c8d9d2144aee

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

diff --git a/FsCrypt.cpp b/FsCrypt.cpp
index 765073d..017ffec 100644
--- a/FsCrypt.cpp
+++ b/FsCrypt.cpp
@@ -186,10 +186,7 @@
     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);
 }

KeyStorage.cpp

diff --git a/KeyStorage.cpp b/KeyStorage.cpp
index af624e3..4893c2f 100644
--- a/KeyStorage.cpp
+++ b/KeyStorage.cpp
@@ -288,9 +288,7 @@
     }
 }
 
-// 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 @@
         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 @@
     }
     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;

KeyStorage.h

diff --git a/KeyStorage.h b/KeyStorage.h
index e318959..de719e9 100644
--- a/KeyStorage.h
+++ b/KeyStorage.h
@@ -41,6 +41,10 @@
 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.