From 634c34ea179cf42507665e9f30d21acba8112992 Mon Sep 17 00:00:00 2001
From: Pavel Grafov <pgrafov@google.com>
Date: Tue, 10 Apr 2018 19:19:01 +0100
Subject: [PATCH] Serialize key eviction vs. user storage preparation

UserManagerService.onBeforeUnlockUser requires unlocked user key
and is executed on FgThread. Currently key may be locked from
a different thread: UserController.finishUserStopped is executed in
mHandler. This changes moves lockUserKey part to FgThread, so that
key state can be reliably checked before starting onBeforeUnlockUser.

In the worst case user will be RUNNING_LOCKED with "Some functionality
may be limited" warning and unable to start apps. But that seems
fairly harmless.

+ got rid of redundant boolean in finishUserStopped.

Fixes: 72334925
Test: Turn work mode on and off ad nauseam
Test cts-tradefed run commandAndExit cts-dev -m CtsDevicePolicyManagerTestCases
     -t com.android.cts.devicepolicy.ManagedProfileTest
Test: creating another user and swithching back and forth
Test: exercising DPMS.lockNow(FLAG_EVICT_CREDENTIAL_ENCRYPTION_KEY) via TestDPC

Change-Id: I01d4dea183fd1a35a2e47284c7a544725e8a871f
---
 .../com/android/server/am/UserController.java | 38 ++++++++++++-------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/services/core/java/com/android/server/am/UserController.java b/services/core/java/com/android/server/am/UserController.java
index 060d4c8497dda..8ad8256225d0f 100644
--- a/services/core/java/com/android/server/am/UserController.java
+++ b/services/core/java/com/android/server/am/UserController.java
@@ -400,6 +400,10 @@ class UserController implements Handler.Callback {
 
         // Call onBeforeUnlockUser on a worker thread that allows disk I/O
         FgThread.getHandler().post(() -> {
+            if (!StorageManager.isUserKeyUnlocked(userId)) {
+                Slog.w(TAG, "User key got locked unexpectedly, leaving user locked.");
+                return;
+            }
             mInjector.getUserManager().onBeforeUnlockUser(userId);
             synchronized (mLock) {
                 // Do not proceed if unexpected state
@@ -714,14 +718,11 @@ class UserController implements Handler.Callback {
 
     void finishUserStopped(UserState uss) {
         final int userId = uss.mHandle.getIdentifier();
-        boolean stopped;
+        final boolean stopped;
         ArrayList<IStopUserCallback> callbacks;
-        boolean forceStopUser = false;
         synchronized (mLock) {
             callbacks = new ArrayList<>(uss.mStopCallbacks);
-            if (mStartedUsers.get(userId) != uss) {
-                stopped = false;
-            } else if (uss.state != UserState.STATE_SHUTDOWN) {
+            if (mStartedUsers.get(userId) != uss || uss.state != UserState.STATE_SHUTDOWN) {
                 stopped = false;
             } else {
                 stopped = true;
@@ -729,10 +730,10 @@ class UserController implements Handler.Callback {
                 mStartedUsers.remove(userId);
                 mUserLru.remove(Integer.valueOf(userId));
                 updateStartedUserArrayLU();
-                forceStopUser = true;
             }
         }
-        if (forceStopUser) {
+
+        if (stopped) {
             mInjector.getUserManagerInternal().removeUserState(userId);
             mInjector.activityManagerOnUserStopped(userId);
             // Clean up all state and processes associated with the user.
@@ -755,12 +756,23 @@ class UserController implements Handler.Callback {
             if (getUserInfo(userId).isEphemeral()) {
                 mInjector.getUserManager().removeUserEvenWhenDisallowed(userId);
             }
-            // Evict the user's credential encryption key.
-            try {
-                getStorageManager().lockUserKey(userId);
-            } catch (RemoteException re) {
-                throw re.rethrowAsRuntimeException();
-            }
+
+            // Evict the user's credential encryption key. Performed on FgThread to make it
+            // serialized with call to UserManagerService.onBeforeUnlockUser in finishUserUnlocking
+            // to prevent data corruption.
+            FgThread.getHandler().post(() -> {
+                synchronized (mLock) {
+                    if (mStartedUsers.get(userId) != null) {
+                        Slog.w(TAG, "User was restarted, skipping key eviction");
+                        return;
+                    }
+                }
+                try {
+                    getStorageManager().lockUserKey(userId);
+                } catch (RemoteException re) {
+                    throw re.rethrowAsRuntimeException();
+                }
+            });
         }
     }