commit 6a0d998380bda306c877a5f4fb4b080cde4f6503
author Janis Danisevskis <jdanis@google.com> Tue Apr 30 15:43:59 2019 -0700
committer Janis Danisevskis <jdanis@google.com> Fri May 03 16:19:06 2019 -0700
tree 0248fe8eeb3810c803df7c6f06d1154b33eecc5d
parent 37896101eae3d88a142e7d659140ff20dc45a807

Fix grants get lost on key upgrade

The upgrade routine used to call KeyStore->del which purges the given
key blob from the keystore including all existing grants.
With this patch, upgrade only calls Keymaster::delete on the keyblobs
without purging it from the keystore. Also it only calls
Keymaster::delete once the upgrade key was successfully written to disk.

This patch also calls fsync on the directory containing keyblobs to
narrow the window in which keyblob may be lost due to power loss.

Bug: 110450771
Test: Upgrade path tested by manually creating a key, bumping the
      patchlevel, using the key subsequently and inspecting the logs.
Change-Id: I89241b5d4033b270733ff61838ab9244fce28c60

keystore/blob.cpp

diff --git a/keystore/blob.cpp b/keystore/blob.cpp
index d1629cb..0987139 100644
--- a/keystore/blob.cpp
+++ b/keystore/blob.cpp
@@ -36,6 +36,7 @@
 #include <string>
 
 #include <android-base/logging.h>
+#include <android-base/unique_fd.h>
 
 namespace {
 
@@ -341,22 +342,35 @@
 
     size_t fileLength = offsetof(blobv3, value) + dataLength + rawBlob->info;
 
-    int out =
-        TEMP_FAILURE_RETRY(open(filename.c_str(), O_WRONLY | O_TRUNC | O_CREAT, S_IRUSR | S_IWUSR));
-    if (out < 0) {
-        ALOGW("could not open file: %s: %s", filename.c_str(), strerror(errno));
+    char tmpFileName[] = ".tmpXXXXXX";
+    {
+        android::base::unique_fd out(TEMP_FAILURE_RETRY(mkstemp(tmpFileName)));
+        if (out < 0) {
+            LOG(ERROR) << "could not open temp file: " << tmpFileName
+                       << " for writing blob file: " << filename.c_str()
+                       << " because: " << strerror(errno);
+            return ResponseCode::SYSTEM_ERROR;
+        }
+
+        const size_t writtenBytes =
+            writeFully(out, reinterpret_cast<uint8_t*>(rawBlob), fileLength);
+
+        if (writtenBytes != fileLength) {
+            LOG(ERROR) << "blob not fully written " << writtenBytes << " != " << fileLength;
+            unlink(tmpFileName);
+            return ResponseCode::SYSTEM_ERROR;
+        }
+    }
+
+    if (rename(tmpFileName, filename.c_str()) == -1) {
+        LOG(ERROR) << "could not rename blob file to " << filename
+                   << " because: " << strerror(errno);
+        unlink(tmpFileName);
         return ResponseCode::SYSTEM_ERROR;
     }
 
-    const size_t writtenBytes = writeFully(out, reinterpret_cast<uint8_t*>(rawBlob), fileLength);
-    if (close(out) != 0) {
-        return ResponseCode::SYSTEM_ERROR;
-    }
-    if (writtenBytes != fileLength) {
-        ALOGW("blob not fully written %zu != %zu", writtenBytes, fileLength);
-        unlink(filename.c_str());
-        return ResponseCode::SYSTEM_ERROR;
-    }
+    fsyncDirectory(getContainingDirectory(filename));
+
     return ResponseCode::NO_ERROR;
 }