commit ba9b7d32a7934f80eced3112857d84b84809d8f3
author Edman Anjos <edmanp@google.com> Wed Sep 02 17:34:54 2020 +0200
committer Edman Anjos <edmanp@google.com> Wed Sep 02 19:11:37 2020 +0200
tree e6511423467cea1d5f8a08bd40ab74fff7820268
parent ca543112cd072bd85a9958c75c9ba5746f7731db

Model KeyStore security level in keymaster worker

On certain device configurations the security level reported by the
Keymaster HAL differs from the security level understood in KeyStore.
Namely, on devices with only a software Keymaster, KeyStore will set it
in the TEE slot, and create a new in-process legacy Keymaster for the
software slot.

This change introduces a field to keymaster worker to represent the
security level that KeyStore understands this worker to operate on.

Bug: 167412989
Test: atest CtsKeystoreTestCases
Change-Id: Ifeaa4782913be45d89cdd175a02302c7dc318719

keystore/KeyStore.cpp

diff --git a/keystore/KeyStore.cpp b/keystore/KeyStore.cpp
index 7545397..1f80899 100644
--- a/keystore/KeyStore.cpp
+++ b/keystore/KeyStore.cpp
@@ -60,8 +60,8 @@
                   "KmasterDevices and KeymasterWorkers must have the same size");
     for (size_t i = 0; i < kmDevices.size(); ++i) {
         if (kmDevices[SecurityLevel(i)]) {
-            mKmDevices[SecurityLevel(i)] =
-                std::make_shared<KeymasterWorker>(kmDevices[SecurityLevel(i)], this);
+            mKmDevices[SecurityLevel(i)] = std::make_shared<KeymasterWorker>(
+                kmDevices[SecurityLevel(i)], this, SecurityLevel(i));
         }
     }
 }

keystore/keymaster_worker.cpp

diff --git a/keystore/keymaster_worker.cpp b/keystore/keymaster_worker.cpp
index 7481a1e..cbb184c 100644
--- a/keystore/keymaster_worker.cpp
+++ b/keystore/keymaster_worker.cpp
@@ -83,8 +83,10 @@
     }
 }
 
-KeymasterWorker::KeymasterWorker(sp<Keymaster> keymasterDevice, KeyStore* keyStore)
-    : keymasterDevice_(std::move(keymasterDevice)), operationMap_(keyStore), keyStore_(keyStore) {
+KeymasterWorker::KeymasterWorker(sp<Keymaster> keymasterDevice, KeyStore* keyStore,
+                                 SecurityLevel internalSecurityLevel)
+    : keymasterDevice_(std::move(keymasterDevice)), operationMap_(keyStore), keyStore_(keyStore),
+      internalSecurityLevel_(internalSecurityLevel) {
     // make sure that hal version is cached.
     if (keymasterDevice_) keymasterDevice_->halVersion();
 }
@@ -821,7 +823,7 @@
             outCharacteristics = keyCharacteristics;
 
             Blob keyBlob(&hidlKeyBlob[0], hidlKeyBlob.size(), nullptr, 0, ::TYPE_KEYMASTER_10);
-            keyBlob.setSecurityLevel(securityLevel);
+            keyBlob.setSecurityLevel(internalSecurityLevel_);
             keyBlob.setCriticalToDeviceEncryption(flags &
                                                   KEYSTORE_FLAG_CRITICAL_TO_DEVICE_ENCRYPTION);
             if (isAuthenticationBound(keyParams) && !keyBlob.isCriticalToDeviceEncryption()) {
@@ -929,7 +931,7 @@
             outCharacteristics = keyCharacteristics;
 
             Blob keyBlob(&hidlKeyBlob[0], hidlKeyBlob.size(), nullptr, 0, ::TYPE_KEYMASTER_10);
-            keyBlob.setSecurityLevel(securityLevel);
+            keyBlob.setSecurityLevel(internalSecurityLevel_);
             keyBlob.setCriticalToDeviceEncryption(flags &
                                                   KEYSTORE_FLAG_CRITICAL_TO_DEVICE_ENCRYPTION);
             if (isAuthenticationBound(keyParams) && !keyBlob.isCriticalToDeviceEncryption()) {
@@ -1004,8 +1006,6 @@
                         CAPTURE_MOVE(worker_cb)]() mutable {
         auto hidlWrappingKey = blob2hidlVec(wrappingBlob);
 
-        SecurityLevel securityLevel = keymasterDevice_->halVersion().securityLevel;
-
         KeyCharacteristics outCharacteristics;
         KeyStoreServiceReturnCode error;
 
@@ -1019,7 +1019,7 @@
             outCharacteristics = keyCharacteristics;
 
             Blob keyBlob(hidlKeyBlob.data(), hidlKeyBlob.size(), nullptr, 0, ::TYPE_KEYMASTER_10);
-            keyBlob.setSecurityLevel(securityLevel);
+            keyBlob.setSecurityLevel(internalSecurityLevel_);
             if (isAuthenticationBound(keyCharacteristics.hardwareEnforced)) {
                 keyBlob.setSuperEncrypted(true);
             }

keystore/keymaster_worker.h

diff --git a/keystore/keymaster_worker.h b/keystore/keymaster_worker.h
index f11af29..fbd52b4 100644
--- a/keystore/keymaster_worker.h
+++ b/keystore/keymaster_worker.h
@@ -135,6 +135,19 @@
     OperationMap operationMap_;
     KeyStore* keyStore_;
 
+    /**
+     * Models the security level of this worker internal to KeyStore.
+     *
+     * When the device has only a software Keymaster, KeyStore will set it on the TEE slot and
+     * instantiate a new in-process software Keymaster. In that case there is a mismatch between the
+     * security level used by KeyStore and what is reported from the HAL. This represents the level
+     * used internally by KeyStore.
+     *
+     * This value is used to associate blobs to the corresponding Keymaster backend. It does not
+     * indicate an actual Keymaster HAL security level and should never be exposed to users.
+     */
+    SecurityLevel internalSecurityLevel_;
+
     template <typename KMFn, typename ErrorType, typename... Args, size_t... I>
     void unwrap_tuple(KMFn kmfn, std::function<void(ErrorType)> cb,
                       const std::tuple<Args...>& tuple, std::index_sequence<I...>) {
@@ -200,7 +213,8 @@
                                          hidl_vec<KeyParameter>* params);
 
   public:
-    KeymasterWorker(sp<Keymaster> keymasterDevice, KeyStore* keyStore);
+    KeymasterWorker(sp<Keymaster> keymasterDevice, KeyStore* keyStore,
+                    SecurityLevel internalSecurityLevel);
 
     void logIfKeymasterVendorError(ErrorCode ec) const;