Merge "Fix keystore wifi concurrency issue." am: dece12434d am: fe8fb1fc2c

am: 9a9794408c

Change-Id: Ic9c42a6876f056f70d25751c9feb2e650f753ac9

2 files changed, 51 insertions, 0 deletions

diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp
index 0bd3e03a..5e7efab0 100644
--- a/keystore/key_store_service.cpp
+++ b/keystore/key_store_service.cpp
@@ -82,6 +82,7 @@ bool containsTag(const hidl_vec<KeyParameter>& params, Tag tag) {
 }
 
 #define AIDL_RETURN(rc) (*_aidl_return = KeyStoreServiceReturnCode(rc).getErrorCode(), Status::ok())
+#define KEYSTORE_SERVICE_LOCK std::lock_guard<std::mutex> keystore_lock(keystoreServiceMutex_)
 
 std::pair<KeyStoreServiceReturnCode, bool> hadFactoryResetSinceIdRotation() {
     struct stat sbuf;
@@ -145,6 +146,7 @@ KeyStoreServiceReturnCode updateParamsForAttestation(uid_t callingUid, Authoriza
 }  // anonymous namespace
 
 Status KeyStoreService::getState(int32_t userId, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_GET_STATE)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -154,6 +156,7 @@ Status KeyStoreService::getState(int32_t userId, int32_t* aidl_return) {
 }
 
 Status KeyStoreService::get(const String16& name, int32_t uid, ::std::vector<uint8_t>* item) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t targetUid = getEffectiveUid(uid);
     if (!checkBinderPermission(P_GET, targetUid)) {
         // see keystore/keystore.h
@@ -183,6 +186,7 @@ Status KeyStoreService::get(const String16& name, int32_t uid, ::std::vector<uin
 
 Status KeyStoreService::insert(const String16& name, const ::std::vector<uint8_t>& item,
                                int targetUid, int32_t flags, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     targetUid = getEffectiveUid(targetUid);
     KeyStoreServiceReturnCode result =
         checkBinderPermissionAndKeystoreState(P_INSERT, targetUid, flags & KEYSTORE_FLAG_ENCRYPTED);
@@ -208,6 +212,7 @@ Status KeyStoreService::insert(const String16& name, const ::std::vector<uint8_t
 }
 
 Status KeyStoreService::del(const String16& name, int targetUid, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     targetUid = getEffectiveUid(targetUid);
     if (!checkBinderPermission(P_DELETE, targetUid)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
@@ -228,6 +233,7 @@ Status KeyStoreService::del(const String16& name, int targetUid, int32_t* aidl_r
 }
 
 Status KeyStoreService::exist(const String16& name, int targetUid, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     targetUid = getEffectiveUid(targetUid);
     if (!checkBinderPermission(P_EXIST, targetUid)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
@@ -243,6 +249,7 @@ Status KeyStoreService::exist(const String16& name, int targetUid, int32_t* aidl
 
 Status KeyStoreService::list(const String16& prefix, int32_t targetUid,
                              ::std::vector<::android::String16>* matches) {
+    KEYSTORE_SERVICE_LOCK;
     targetUid = getEffectiveUid(targetUid);
     if (!checkBinderPermission(P_LIST, targetUid)) {
         return Status::fromServiceSpecificError(
@@ -281,6 +288,7 @@ Status KeyStoreService::list(const String16& prefix, int32_t targetUid,
  */
 Status KeyStoreService::listUidsOfAuthBoundKeys(std::vector<std::string>* uidsOut,
                                                 int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     const int32_t callingUid = IPCThreadState::self()->getCallingUid();
     const int32_t userId = get_user_id(callingUid);
     const int32_t appId = get_app_id(callingUid);
@@ -344,6 +352,7 @@ Status KeyStoreService::listUidsOfAuthBoundKeys(std::vector<std::string>* uidsOu
 }
 
 Status KeyStoreService::reset(int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_RESET)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -357,6 +366,7 @@ Status KeyStoreService::reset(int32_t* aidl_return) {
 
 Status KeyStoreService::onUserPasswordChanged(int32_t userId, const String16& password,
                                               int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_PASSWORD)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -394,6 +404,7 @@ Status KeyStoreService::onUserPasswordChanged(int32_t userId, const String16& pa
 }
 
 Status KeyStoreService::onUserAdded(int32_t userId, int32_t parentId, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_USER_CHANGED)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -419,6 +430,7 @@ Status KeyStoreService::onUserAdded(int32_t userId, int32_t parentId, int32_t* a
 }
 
 Status KeyStoreService::onUserRemoved(int32_t userId, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_USER_CHANGED)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -430,6 +442,7 @@ Status KeyStoreService::onUserRemoved(int32_t userId, int32_t* aidl_return) {
 }
 
 Status KeyStoreService::lock(int32_t userId, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_LOCK)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -449,6 +462,7 @@ Status KeyStoreService::lock(int32_t userId, int32_t* aidl_return) {
 }
 
 Status KeyStoreService::unlock(int32_t userId, const String16& pw, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_UNLOCK)) {
         *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
         return Status::ok();
@@ -479,6 +493,7 @@ Status KeyStoreService::unlock(int32_t userId, const String16& pw, int32_t* aidl
 }
 
 Status KeyStoreService::isEmpty(int32_t userId, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkBinderPermission(P_IS_EMPTY)) {
         *aidl_return = static_cast<int32_t>(false);
         return Status::ok();
@@ -490,6 +505,7 @@ Status KeyStoreService::isEmpty(int32_t userId, int32_t* aidl_return) {
 
 Status KeyStoreService::grant(const String16& name, int32_t granteeUid,
                               ::android::String16* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
     auto result =
         checkBinderPermissionAndKeystoreState(P_GRANT, /*targetUid=*/-1, /*checkUnlocked=*/false);
@@ -510,6 +526,7 @@ Status KeyStoreService::grant(const String16& name, int32_t granteeUid,
 }
 
 Status KeyStoreService::ungrant(const String16& name, int32_t granteeUid, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
     KeyStoreServiceReturnCode result =
         checkBinderPermissionAndKeystoreState(P_GRANT, /*targetUid=*/-1, /*checkUnlocked=*/false);
@@ -530,6 +547,7 @@ Status KeyStoreService::ungrant(const String16& name, int32_t granteeUid, int32_
 }
 
 Status KeyStoreService::getmtime(const String16& name, int32_t uid, int64_t* time) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t targetUid = getEffectiveUid(uid);
     if (!checkBinderPermission(P_GET, targetUid)) {
         ALOGW("permission denied for %d: getmtime", targetUid);
@@ -568,11 +586,13 @@ Status KeyStoreService::getmtime(const String16& name, int32_t uid, int64_t* tim
 }
 
 Status KeyStoreService::is_hardware_backed(const String16& keyType, int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     *aidl_return = static_cast<int32_t>(mKeyStore->isHardwareBacked(keyType) ? 1 : 0);
     return Status::ok();
 }
 
 Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t targetUid = getEffectiveUid(targetUid64);
     if (!checkBinderPermissionSelfOrSystem(P_CLEAR_UID, targetUid)) {
         return AIDL_RETURN(ResponseCode::PERMISSION_DENIED);
@@ -612,6 +632,7 @@ Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* _aidl_return) {
 Status KeyStoreService::addRngEntropy(
     const ::android::sp<::android::security::keystore::IKeystoreResponseCallback>& cb,
     const ::std::vector<uint8_t>& entropy, int32_t flags, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     auto device = mKeyStore->getDevice(flagsToSecurityLevel(flags));
     if (!device) {
         return AIDL_RETURN(ErrorCode::HARDWARE_TYPE_UNAVAILABLE);
@@ -628,6 +649,7 @@ Status KeyStoreService::generateKey(
     const ::android::sp<::android::security::keystore::IKeystoreKeyCharacteristicsCallback>& cb,
     const String16& name, const KeymasterArguments& params, const ::std::vector<uint8_t>& entropy,
     int uid, int flags, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     // TODO(jbires): remove this getCallingUid call upon implementation of b/25646100
     uid_t originalUid = IPCThreadState::self()->getCallingUid();
     uid = getEffectiveUid(uid);
@@ -689,6 +711,7 @@ Status KeyStoreService::getKeyCharacteristics(
     const String16& name, const ::android::security::keymaster::KeymasterBlob& clientId,
     const ::android::security::keymaster::KeymasterBlob& appData, int32_t uid,
     int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
 
     uid_t targetUid = getEffectiveUid(uid);
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
@@ -735,6 +758,7 @@ Status KeyStoreService::importKey(
     const ::android::sp<::android::security::keystore::IKeystoreKeyCharacteristicsCallback>& cb,
     const String16& name, const KeymasterArguments& params, int32_t format,
     const ::std::vector<uint8_t>& keyData, int uid, int flags, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     uid = getEffectiveUid(uid);
     auto logOnScopeExit = android::base::make_scope_guard([&] {
         if (__android_log_security()) {
@@ -791,6 +815,7 @@ Status KeyStoreService::exportKey(
     const ::android::security::keymaster::KeymasterBlob& clientId,
     const ::android::security::keymaster::KeymasterBlob& appData, int32_t uid,
     int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
 
     uid_t targetUid = getEffectiveUid(uid);
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
@@ -826,6 +851,7 @@ Status KeyStoreService::begin(const sp<IKeystoreOperationResultCallback>& cb,
                               bool pruneable, const KeymasterArguments& params,
                               const ::std::vector<uint8_t>& entropy, int32_t uid,
                               int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
     uid_t targetUid = getEffectiveUid(uid);
     if (!is_granted_to(callingUid, targetUid)) {
@@ -874,6 +900,7 @@ Status KeyStoreService::update(const ::android::sp<IKeystoreOperationResultCallb
                                const ::android::sp<::android::IBinder>& token,
                                const ::android::security::keymaster::KeymasterArguments& params,
                                const ::std::vector<uint8_t>& input, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkAllowedOperationParams(params.getParameters())) {
         return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT);
     }
@@ -898,6 +925,7 @@ Status KeyStoreService::finish(const ::android::sp<IKeystoreOperationResultCallb
                                const ::android::security::keymaster::KeymasterArguments& params,
                                const ::std::vector<uint8_t>& signature,
                                const ::std::vector<uint8_t>& entropy, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     if (!checkAllowedOperationParams(params.getParameters())) {
         return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT);
     }
@@ -921,6 +949,7 @@ Status KeyStoreService::finish(const ::android::sp<IKeystoreOperationResultCallb
 Status KeyStoreService::abort(const ::android::sp<IKeystoreResponseCallback>& cb,
                               const ::android::sp<::android::IBinder>& token,
                               int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     auto dev = getOperationDevice(token);
     if (!dev) {
         return AIDL_RETURN(ErrorCode::INVALID_OPERATION_HANDLE);
@@ -933,6 +962,7 @@ Status KeyStoreService::abort(const ::android::sp<IKeystoreResponseCallback>& cb
 
 Status KeyStoreService::addAuthToken(const ::std::vector<uint8_t>& authTokenAsVector,
                                      int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
 
     // TODO(swillden): When gatekeeper and fingerprint are ready, this should be updated to
     // receive a HardwareAuthToken, rather than an opaque byte array.
@@ -983,6 +1013,7 @@ bool isDeviceIdAttestationRequested(const KeymasterArguments& params) {
 Status KeyStoreService::attestKey(
     const ::android::sp<::android::security::keystore::IKeystoreCertificateChainCallback>& cb,
     const String16& name, const KeymasterArguments& params, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     // check null output if method signature is updated and return ErrorCode::OUTPUT_PARAMETER_NULL
     if (!checkAllowedOperationParams(params.getParameters())) {
         return AIDL_RETURN(ErrorCode::INVALID_ARGUMENT);
@@ -1038,6 +1069,7 @@ Status KeyStoreService::attestKey(
 Status KeyStoreService::attestDeviceIds(
     const ::android::sp<::android::security::keystore::IKeystoreCertificateChainCallback>& cb,
     const KeymasterArguments& params, int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     // check null output if method signature is updated and return ErrorCode::OUTPUT_PARAMETER_NULL
 
     if (!checkAllowedOperationParams(params.getParameters())) {
@@ -1130,6 +1162,7 @@ Status KeyStoreService::attestDeviceIds(
 }
 
 Status KeyStoreService::onDeviceOffBody(int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     // TODO(tuckeris): add permission check.  This should be callable from ClockworkHome only.
     mKeyStore->getAuthTokenTable().onDeviceOffBody();
     *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR);
@@ -1142,6 +1175,7 @@ Status KeyStoreService::importWrappedKey(
     const ::android::String16& wrappingKeyAlias, const ::std::vector<uint8_t>& maskingKey,
     const KeymasterArguments& params, int64_t rootSid, int64_t fingerprintSid,
     int32_t* _aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
 
     uid_t callingUid = IPCThreadState::self()->getCallingUid();
 
@@ -1191,16 +1225,19 @@ Status KeyStoreService::presentConfirmationPrompt(const sp<IBinder>& listener,
                                                   const ::std::vector<uint8_t>& extraData,
                                                   const String16& locale, int32_t uiOptionsAsFlags,
                                                   int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     return mKeyStore->getConfirmationManager().presentConfirmationPrompt(
         listener, promptText, extraData, locale, uiOptionsAsFlags, aidl_return);
 }
 
 Status KeyStoreService::cancelConfirmationPrompt(const sp<IBinder>& listener,
                                                  int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     return mKeyStore->getConfirmationManager().cancelConfirmationPrompt(listener, aidl_return);
 }
 
 Status KeyStoreService::isConfirmationPromptSupported(bool* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     return mKeyStore->getConfirmationManager().isConfirmationPromptSupported(aidl_return);
 }
 
@@ -1318,6 +1355,7 @@ bool KeyStoreService::checkAllowedOperationParams(const hidl_vec<KeyParameter>&
 
 Status KeyStoreService::onKeyguardVisibilityChanged(bool isShowing, int32_t userId,
                                                     int32_t* aidl_return) {
+    KEYSTORE_SERVICE_LOCK;
     mKeyStore->getEnforcementPolicy().set_device_locked(isShowing, userId);
     *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR);
 
diff --git a/keystore/key_store_service.h b/keystore/key_store_service.h
index 2171213c..2fdc3dd2 100644
--- a/keystore/key_store_service.h
+++ b/keystore/key_store_service.h
@@ -35,6 +35,8 @@
 #include <keystore/OperationResult.h>
 #include <keystore/keystore_return_types.h>
 
+#include <mutex>
+
 namespace keystore {
 
 // Class provides implementation for generated BnKeystoreService.h based on
@@ -230,6 +232,17 @@ class KeyStoreService : public android::security::keystore::BnKeystoreService {
 
     sp<KeyStore> mKeyStore;
 
+    /**
+     * This mutex locks keystore operations from concurrent execution.
+     * The keystore service has always been conceptually single threaded. Even with the introduction
+     * of keymaster workers, it was assumed that the dispatcher thread executes exclusively on
+     * certain code paths. With the introduction of wifi Keystore service in the keystore process
+     * this assumption no longer holds as the hwbinder thread servicing this interface makes
+     * functions (rather than IPC) calls into keystore. This mutex protects the keystore logic
+     * from concurrent execution.
+     */
+    std::mutex keystoreServiceMutex_;
+
     std::mutex operationDeviceMapMutex_;
     std::map<sp<IBinder>, std::shared_ptr<KeymasterWorker>> operationDeviceMap_;