Fix deadlock in keystore
The keystore dispatcher thread must not hold any locks when it calls
LockedKeyBlobEntry::list.
Also fixed an issue with the KeyBlobEntry conparisson function that lead
to clients beeing blockd by each other if they edit the same alias at
the same time altough they have different name spaces.
Test: A script that is currently under development. Stay tuned
Change-Id: I1d7f46cc27b3d90305a0ea64b8859d85b42996d8
diff --git a/keystore/KeyStore.cpp b/keystore/KeyStore.cpp
index 7530243..ac3ab5f 100644
--- a/keystore/KeyStore.cpp
+++ b/keystore/KeyStore.cpp
@@ -138,21 +138,13 @@
android::String8 prefix("");
android::Vector<android::String16> aliases;
- // DO NOT
- // move
- // auto userState = userStateDB_.getUserState(userId);
- // here, in an attempt to replace userStateDB_.getUserState(userId) with userState.
- // userState is a proxy that holds a lock which may required by a worker.
- // LockedKeyBlobEntry::list has a fence that waits until all workers have finished which may
- // not happen if a user state lock is held. The following line only briefly grabs the lock.
- // Grabbing the user state lock after the list call is also save since workers cannot grab
- // blob entry locks.
-
auto userState = mUserStateDB.getUserState(userId);
std::string userDirName = userState->getUserDirName();
auto encryptionKey = userState->getEncryptionKey();
auto state = userState->getState();
- // unlock the user state
+ // userState is a proxy that holds a lock which may be required by a worker.
+ // LockedKeyBlobEntry::list has a fence that waits until all workers have finished which may
+ // not happen if a user state lock is held. The following line relinquishes the lock.
userState = {};
ResponseCode rc;
@@ -217,7 +209,7 @@
bool KeyStore::isEmpty(uid_t userId) const {
std::string userDirName;
{
- // userState hold a lock which must be relinqhished before list is called. This scope
+ // userState holds a lock which must be relinquished before list is called. This scope
// prevents deadlocks.
auto userState = mUserStateDB.getUserState(userId);
if (!userState) {
diff --git a/keystore/blob.h b/keystore/blob.h
index 5cd1b90..a7f9fd0 100644
--- a/keystore/blob.h
+++ b/keystore/blob.h
@@ -196,10 +196,10 @@
bool hasCharacteristicsBlob() const;
bool operator<(const KeyBlobEntry& rhs) const {
- return std::tie(alias_, user_dir_, uid_) < std::tie(rhs.alias_, rhs.user_dir_, uid_);
+ return std::tie(uid_, alias_, user_dir_) < std::tie(rhs.uid_, rhs.alias_, rhs.user_dir_);
}
bool operator==(const KeyBlobEntry& rhs) const {
- return std::tie(alias_, user_dir_, uid_) == std::tie(rhs.alias_, rhs.user_dir_, uid_);
+ return std::tie(uid_, alias_, user_dir_) == std::tie(rhs.uid_, rhs.alias_, rhs.user_dir_);
}
bool operator!=(const KeyBlobEntry& rhs) const { return !(*this == rhs); }
diff --git a/keystore/key_store_service.cpp b/keystore/key_store_service.cpp
index 24bf331..fa010e6 100644
--- a/keystore/key_store_service.cpp
+++ b/keystore/key_store_service.cpp
@@ -256,10 +256,10 @@
ResponseCode rc;
std::list<LockedKeyBlobEntry> internal_matches;
+ auto userDirName = mKeyStore->getUserStateDB().getUserStateByUid(targetUid)->getUserDirName();
- std::tie(rc, internal_matches) = LockedKeyBlobEntry::list(
- mKeyStore->getUserStateDB().getUserStateByUid(targetUid)->getUserDirName(),
- [&](uid_t uid, const std::string& alias) {
+ std::tie(rc, internal_matches) =
+ LockedKeyBlobEntry::list(userDirName, [&](uid_t uid, const std::string& alias) {
std::mismatch(stdPrefix.begin(), stdPrefix.end(), alias.begin(), alias.end());
return uid == static_cast<uid_t>(targetUid) &&
std::mismatch(stdPrefix.begin(), stdPrefix.end(), alias.begin(), alias.end())
@@ -582,11 +582,10 @@
return Status::ok();
}
-Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* aidl_return) {
+Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* _aidl_return) {
uid_t targetUid = getEffectiveUid(targetUid64);
if (!checkBinderPermissionSelfOrSystem(P_CLEAR_UID, targetUid)) {
- *aidl_return = static_cast<int32_t>(ResponseCode::PERMISSION_DENIED);
- return Status::ok();
+ return AIDL_RETURN(ResponseCode::PERMISSION_DENIED);
}
ALOGI("clear_uid %" PRId64, targetUid64);
@@ -594,16 +593,15 @@
ResponseCode rc;
std::list<LockedKeyBlobEntry> entries;
+ auto userDirName = mKeyStore->getUserStateDB().getUserStateByUid(targetUid)->getUserDirName();
// list has a fence making sure no workers are modifying blob files before iterating the
// data base. All returned entries are locked.
std::tie(rc, entries) = LockedKeyBlobEntry::list(
- mKeyStore->getUserStateDB().getUserStateByUid(targetUid)->getUserDirName(),
- [&](uid_t uid, const std::string&) -> bool { return uid == targetUid; });
+ userDirName, [&](uid_t uid, const std::string&) -> bool { return uid == targetUid; });
if (rc != ResponseCode::NO_ERROR) {
- *aidl_return = static_cast<int32_t>(rc);
- return Status::ok();
+ return AIDL_RETURN(rc);
}
for (LockedKeyBlobEntry& lockedEntry : entries) {
@@ -618,8 +616,7 @@
}
mKeyStore->del(lockedEntry);
}
- *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR);
- return Status::ok();
+ return AIDL_RETURN(ResponseCode::NO_ERROR);
}
Status KeyStoreService::addRngEntropy(
diff --git a/keystore/keymaster_worker.cpp b/keystore/keymaster_worker.cpp
index f3bf71f..6dc055f 100644
--- a/keystore/keymaster_worker.cpp
+++ b/keystore/keymaster_worker.cpp
@@ -88,6 +88,8 @@
std::tie(rc, blob, charBlob) =
lockedEntry.readBlobs(userState->getEncryptionKey(), userState->getState());
+ userState = {};
+
if (rc != ResponseCode::NO_ERROR) {
return error = rc, result;
}