Merge changes from topic "rename-key-dir"

* changes:
  Don't export storeKey(), and update comments
  Always use RenameKeyDir() when moving/renaming key directories
  Make RenameKeyDir() use IsSameFile()

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) {
+        if (!android::vold::RenameKeyDir(to_fix, current_path)) return;
-            PLOG(WARNING) << "Unable to rename " << to_fix << " to " << current_path;
-            return;
-        }
     }
     android::vold::FsyncDirectory(directory_path);
 }

KeyStorage.cpp

@@ -216,6 +216,7 @@ 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.
+// A directory can be in this list at most once.
 static std::vector<std::string> key_dirs_to_commit;
 
 // Replaces |dir|/keymaster_key_blob with |dir|/keymaster_key_blob_upgraded and
@@ -267,8 +268,9 @@ static bool IsKeyCommitPending(const std::string& dir) {
     return false;
 }
 
-// Schedules the upgraded Keymaster key in |dir| to be committed later.
+// Schedules the upgraded Keymaster key in |dir| to be committed later.  Assumes
-// Assumes that key_upgrade_lock is held.
+// 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);
@@ -286,21 +288,25 @@ static void CancelPendingKeyCommit(const std::string& dir) {
     }
 }
 
-// Renames a key directory. Also updates the deferred commit vector
+bool RenameKeyDir(const std::string& old_name, const std::string& new_name) {
-// (key_dirs_to_commit) appropriately.
-//
-// However, @old_name must be the path to the directory that was used to put that
-// directory into the deferred commit list in the first place (since this function
-// directly compares paths instead of using IsSameFile()).
-static bool RenameKeyDir(const std::string& old_name, const std::string& new_name) {
     std::lock_guard<std::mutex> lock(key_upgrade_lock);
 
-    if (rename(old_name.c_str(), new_name.c_str()) != 0) return false;
+    // Find the entry in key_dirs_to_commit (if any) for this directory so that
-
+    // we can update it if the rename succeeds.  We don't allow duplicates in
-    // IsSameFile() doesn't work here since we just renamed @old_name.
+    // this list, so there can be at most one such entry.
-    for (auto it = key_dirs_to_commit.begin(); it != key_dirs_to_commit.end(); it++) {
+    auto it = key_dirs_to_commit.begin();
-        if (*it == old_name) *it = new_name;
+    for (; it != key_dirs_to_commit.end(); it++) {
+        if (IsSameFile(old_name, *it)) break;
     }
+
+    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;
+
     return true;
 }
 
@@ -569,7 +575,12 @@ static bool decryptWithoutKeymaster(const std::string& preKey, const std::string
     return true;
 }
 
-bool storeKey(const std::string& dir, const KeyAuthentication& auth, const KeyBuffer& key) {
+// Creates a directory at the given path |dir| and stores |key| in it, in such a
+// way that it can only be retrieved via Keymaster (if no secret is given in
+// |auth|) or with the given secret (if a secret is given in |auth|), and can be
+// securely deleted.  If a storage binding seed has been set, then the storage
+// binding seed will be required to retrieve the key as well.
+static bool storeKey(const std::string& dir, const KeyAuthentication& auth, const KeyBuffer& key) {
     if (TEMP_FAILURE_RETRY(mkdir(dir.c_str(), 0700)) == -1) {
         PLOG(ERROR) << "key mkdir " << dir;
         return false;
@@ -610,10 +621,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)) {
+    if (!RenameKeyDir(tmp_path, key_path)) return false;
-        PLOG(ERROR) << "Unable to move new key to location: " << key_path;
+
-        return false;
-    }
     if (!FsyncParentDirectory(key_path)) return false;
     LOG(DEBUG) << "Created key: " << key_path;
     return true;

KeyStorage.h

@@ -41,11 +41,9 @@ extern const KeyAuthentication kEmptyAuthentication;
 bool createSecdiscardable(const std::string& path, std::string* hash);
 bool readSecdiscardable(const std::string& path, std::string* hash);
 
-// Create a directory at the named path, and store "key" in it,
+// Renames a key directory while also managing deferred commits appropriately.
-// in such a way that it can only be retrieved via Keymaster and
+// This method should be used whenever a key directory needs to be moved/renamed.
-// can be securely deleted.
+bool RenameKeyDir(const std::string& old_name, const std::string& new_name);
-// It's safe to move/rename the directory after creation.
-bool storeKey(const std::string& dir, const KeyAuthentication& auth, const KeyBuffer& key);
 
 // Create a directory at the named path, and store "key" in it as storeKey
 // This version creates the key in "tmp_path" then atomically renames "tmp_path"