Fix keystore wifi concurrency issue.
Keystore was conceptually single threaded. Even with the introduction of
Keymaster workers it was always assumed that the service dispatcher
thread was single threaded. The wifi keystore service, however, calls
into the keystore service concurrently.
This patch adds a lock around all keystore service entry points to make
sure all dispatcher executions are serialised despite being called from
both the binder and hwbinder service thread.
Bug: 128810613
Bug: 129145334
Bug: 128774635
Test: Regressions tested with Keystore CTS test suite.
Change-Id: I8c5602d2c2cb1dd9423df713037e99b247cee71f
diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp
index a7fcd38..8efc7c7 100644
--- a/keystore/key_store_service.cpp
+++ b/keystore/key_store_service.cpp
@@ -84,6 +84,7 @@
}
#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;
@@ -147,6 +148,7 @@
} // 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();
@@ -156,6 +158,7 @@
}
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
@@ -185,6 +188,7 @@
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);
@@ -210,6 +214,7 @@
}
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);
@@ -230,6 +235,7 @@
}
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);
@@ -245,6 +251,7 @@
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(
@@ -283,6 +290,7 @@
*/
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);
@@ -346,6 +354,7 @@
}
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();
@@ -359,6 +368,7 @@
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();
@@ -400,6 +410,7 @@
}
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();
@@ -425,6 +436,7 @@
}
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();
@@ -436,6 +448,7 @@
}
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();
@@ -455,6 +468,7 @@
}
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();
@@ -485,6 +499,7 @@
}
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();
@@ -496,6 +511,7 @@
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);
@@ -516,6 +532,7 @@
}
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);
@@ -536,6 +553,7 @@
}
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);
@@ -574,11 +592,13 @@
}
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);
@@ -618,6 +638,7 @@
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);
@@ -634,6 +655,7 @@
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);
@@ -695,6 +717,7 @@
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();
@@ -741,6 +764,7 @@
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()) {
@@ -797,6 +821,7 @@
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();
@@ -832,6 +857,7 @@
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)) {
@@ -880,6 +906,7 @@
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);
}
@@ -904,6 +931,7 @@
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);
}
@@ -927,6 +955,7 @@
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);
@@ -939,6 +968,7 @@
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.
@@ -993,6 +1023,7 @@
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);
@@ -1054,6 +1085,7 @@
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())) {
@@ -1144,6 +1176,7 @@
}
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);
@@ -1156,6 +1189,7 @@
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();
@@ -1205,16 +1239,19 @@
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);
}
@@ -1330,6 +1367,7 @@
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 2171213..2fdc3dd 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 @@
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_;