Merge changes I096b7c98,I1d7f46cc

* changes:
  Fixed nullptr deref when exporting a non existing key
  Fix deadlock in keystore

keystore/KeyStore.cpp

@@ -138,21 +138,13 @@ void KeyStore::resetUser(uid_t userId, bool keepUnenryptedEntries) {
     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 @@ void KeyStore::resetUser(uid_t userId, bool keepUnenryptedEntries) {
 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) {

keystore/blob.h

@@ -196,10 +196,10 @@ class KeyBlobEntry {
     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); }
 

keystore/key_store_service.cpp

@@ -256,10 +256,10 @@ Status KeyStoreService::list(const String16& prefix, int32_t targetUid,
 
     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 @@ Status KeyStoreService::is_hardware_backed(const String16& keyType, int32_t* aid
     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 @@ Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* aidl_return) {
 
     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 @@ Status KeyStoreService::clear_uid(int64_t targetUid64, int32_t* aidl_return) {
         }
         mKeyStore->del(lockedEntry);
     }
-    *aidl_return = static_cast<int32_t>(ResponseCode::NO_ERROR);
-    return Status::ok();
+    return AIDL_RETURN(ResponseCode::NO_ERROR);
 }
 
 Status KeyStoreService::addRngEntropy(
@@ -821,7 +818,7 @@ Status KeyStoreService::exportKey(
 
     std::tie(rc, keyBlob, charBlob, lockedEntry) =
         mKeyStore->getKeyForName(name8, targetUid, TYPE_KEYMASTER_10);
-    if (!rc) {
+    if (!rc.isOk()) {
         return AIDL_RETURN(rc);
     }
 

keystore/keymaster_worker.cpp

@@ -88,6 +88,8 @@ KeymasterWorker::upgradeKeyBlob(const LockedKeyBlobEntry& lockedEntry,
     std::tie(rc, blob, charBlob) =
         lockedEntry.readBlobs(userState->getEncryptionKey(), userState->getState());
 
+    userState = {};
+
     if (rc != ResponseCode::NO_ERROR) {
         return error = rc, result;
     }