From 567e7a0476a3251d02705932268cd5f395ef863f Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 13 Aug 2021 13:37:55 -0700 Subject: [PATCH 1/5] StorageManagerService: don't ignore failures to prepare user storage We must never leave directories unencrypted. Bug: 164488924 Bug: 224585613 Change-Id: I9a38ab5cca1ae9c9ebff81fca04615fd83ebe4b2 (cherry picked from commit 50946dd15fd14cbf92b5c7e32ac7a0f088b8b302) Merged-In: I9a38ab5cca1ae9c9ebff81fca04615fd83ebe4b2 --- .../core/java/com/android/server/StorageManagerService.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 69c29269b7a99..22a3b338521e1 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3397,8 +3397,12 @@ class StorageManagerService extends IStorageManager.Stub mInstaller.tryMountDataMirror(volumeUuid); } } - } catch (Exception e) { + } catch (RemoteException | Installer.InstallerException e) { Slog.wtf(TAG, e); + // Make sure to re-throw this exception; we must not ignore failure + // to prepare the user storage as it could indicate that encryption + // wasn't successfully set up. + throw new RuntimeException(e); } } From e1f17026ca80e43952fcc5d3a246615b711eba0a Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 24 Jan 2022 20:33:11 +0000 Subject: [PATCH 2/5] UserDataPreparer: reboot to recovery if preparing user storage fails StorageManager.prepareUserStorage() can throw an exception if a directory cannot be encrypted, for example due to already being nonempty. In this case, usage of the directory must not be allowed to proceed. UserDataPreparer currently handles this by deleting the user's directories, but the error is still ultimately suppressed and starting the user is still allowed to proceed. The correct behavior in this case is to reboot into recovery to ask the user to factory reset the device. This is already what happens when 'init' fails to encrypt a directory with the system DE policy. However, this was overlooked for the user directories. Start doing this. Bug: 164488924 Bug: 224585613 Change-Id: Ib5e91d2510b25780d7a161b91b5cee2f6f7a2e54 (cherry picked from commit 5256365e65882b81509ec2f6b9dfe2dcf0025254) Merged-In: Ib5e91d2510b25780d7a161b91b5cee2f6f7a2e54 --- .../core/java/com/android/server/pm/UserDataPreparer.java | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java index 045a295da965f..504769064808c 100644 --- a/services/core/java/com/android/server/pm/UserDataPreparer.java +++ b/services/core/java/com/android/server/pm/UserDataPreparer.java @@ -22,6 +22,7 @@ import android.content.Context; import android.content.pm.UserInfo; import android.os.Environment; import android.os.FileUtils; +import android.os.RecoverySystem; import android.os.storage.StorageManager; import android.os.storage.VolumeInfo; import android.os.SystemProperties; @@ -115,6 +116,13 @@ class UserDataPreparer { // Try one last time; if we fail again we're really in trouble prepareUserDataLI(volumeUuid, userId, userSerial, flags | StorageManager.FLAG_STORAGE_DE, false); + } else { + try { + Log.e(TAG, "prepareUserData failed", e); + RecoverySystem.rebootPromptAndWipeUserData(mContext, "prepareUserData failed"); + } catch (IOException e2) { + throw new RuntimeException("error rebooting into recovery", e2); + } } } } From fd31f740ce5e7beb342e0e21b983de1100782bdb Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 4 Mar 2022 00:07:29 +0000 Subject: [PATCH 3/5] UserDataPreparer: reboot to recovery for system user only With the next CL, old devices might contain a combination of old users with prepareUserStorage error checking disabled and new users with prepareUserStorage error checking enabled. Factory resetting the whole device when any user fails to prepare may be too aggressive. Also, UserDataPreparer already destroys the affected user's storage when it fails to prepare, which seems to be fairly effective at breaking things for that user (absent proper error handling by upper layers). Therefore, let's only factory reset the device if the failing user is the system user. Bug: 164488924 Bug: 224585613 Change-Id: Ia1db01ab4ec6b3b17d725f391c3500d92aa00f97 (cherry picked from commit 4c76da76c9831266e4e63c0618150bed10a929a7) Merged-In: Ia1db01ab4ec6b3b17d725f391c3500d92aa00f97 --- .../core/java/com/android/server/pm/UserDataPreparer.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/pm/UserDataPreparer.java b/services/core/java/com/android/server/pm/UserDataPreparer.java index 504769064808c..95482d7c7f1aa 100644 --- a/services/core/java/com/android/server/pm/UserDataPreparer.java +++ b/services/core/java/com/android/server/pm/UserDataPreparer.java @@ -118,8 +118,11 @@ class UserDataPreparer { flags | StorageManager.FLAG_STORAGE_DE, false); } else { try { - Log.e(TAG, "prepareUserData failed", e); - RecoverySystem.rebootPromptAndWipeUserData(mContext, "prepareUserData failed"); + Log.wtf(TAG, "prepareUserData failed for user " + userId, e); + if (userId == UserHandle.USER_SYSTEM) { + RecoverySystem.rebootPromptAndWipeUserData(mContext, + "prepareUserData failed for system user"); + } } catch (IOException e2) { throw new RuntimeException("error rebooting into recovery", e2); } From e667b604021e543436aabece21dc78d19fe9948e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 4 Mar 2022 00:07:43 +0000 Subject: [PATCH 4/5] Ignore errors preparing user storage for existing users Unfortunately we can't rule out the existence of devices where the user storage wasn't properly prepared, due to StorageManagerService previously ignoring errors from mVold.prepareUserStorage, combined with OEMs potentially creating files in per-user directories too early. And forcing these broken devices to be factory reset upon taking an OTA is not currently considered to be acceptable. One option is to only check for prepareUserStorage errors on devices that launched with T or later. However, this is a serious issue and it would be strongly preferable to do more than that. Therefore, this CL makes it so that errors are checked for all new users, rather than all new devices. A field ignorePrepareStorageErrors is added to the user record; it is only ever set to true implicitly, when reading a user record from disk that lacks this field. This field is used by StorageManagerService to decide whether to check for errors. Bug: 164488924 Bug: 224585613 Test: Intentionally made a device affected by this issue by reverting the CLs that introduced the error checks, and changing vold to inject an error into prepareUserStorage. Then, flashed a build with this CL without wiping userdata. The device still boots, as expected, and the log shows that the error was intentionally ignored. Tested that if a second user is added, the error is *not* ignored and the second user's storage is destroyed before it can be used. Finally, wiped the device and verified that it won't boot up anymore, as expected since error checking is enabled for the system user in that case. Change-Id: I9bdd1a4bf5b14542adb901f264a91d489115c89b (cherry picked from commit 60d8318c47b7b659716d71243d087b34ab327f64) Merged-In: I9bdd1a4bf5b14542adb901f264a91d489115c89b --- .../android/server/StorageManagerService.java | 11 ++++- .../server/pm/UserManagerInternal.java | 8 ++++ .../android/server/pm/UserManagerService.java | 42 +++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 22a3b338521e1..19596ba142cdb 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -3397,11 +3397,20 @@ class StorageManagerService extends IStorageManager.Stub mInstaller.tryMountDataMirror(volumeUuid); } } - } catch (RemoteException | Installer.InstallerException e) { + } catch (Exception e) { Slog.wtf(TAG, e); // Make sure to re-throw this exception; we must not ignore failure // to prepare the user storage as it could indicate that encryption // wasn't successfully set up. + // + // Very unfortunately, these errors need to be ignored for broken + // users that already existed on-disk from older Android versions. + UserManagerInternal umInternal = LocalServices.getService(UserManagerInternal.class); + if (umInternal.shouldIgnorePrepareStorageErrors(userId)) { + Slog.wtf(TAG, "ignoring error preparing storage for existing user " + userId + + "; device may be insecure!"); + return; + } throw new RuntimeException(e); } } diff --git a/services/core/java/com/android/server/pm/UserManagerInternal.java b/services/core/java/com/android/server/pm/UserManagerInternal.java index eb2de60127451..0e6d5e5ed4639 100644 --- a/services/core/java/com/android/server/pm/UserManagerInternal.java +++ b/services/core/java/com/android/server/pm/UserManagerInternal.java @@ -312,4 +312,12 @@ public abstract class UserManagerInternal { */ public abstract void setDefaultCrossProfileIntentFilters( @UserIdInt int parentUserId, @UserIdInt int profileUserId); + + /** + * Returns {@code true} if the system should ignore errors when preparing + * the storage directories for the user with ID {@code userId}. This will + * return {@code false} for all new users; it will only return {@code true} + * for users that already existed on-disk from an older version of Android. + */ + public abstract boolean shouldIgnorePrepareStorageErrors(int userId); } diff --git a/services/core/java/com/android/server/pm/UserManagerService.java b/services/core/java/com/android/server/pm/UserManagerService.java index d4feb3a728c85..8334d537bbf98 100644 --- a/services/core/java/com/android/server/pm/UserManagerService.java +++ b/services/core/java/com/android/server/pm/UserManagerService.java @@ -203,6 +203,8 @@ public class UserManagerService extends IUserManager.Stub { private static final String TAG_SEED_ACCOUNT_OPTIONS = "seedAccountOptions"; private static final String TAG_LAST_REQUEST_QUIET_MODE_ENABLED_CALL = "lastRequestQuietModeEnabledCall"; + private static final String TAG_IGNORE_PREPARE_STORAGE_ERRORS = + "ignorePrepareStorageErrors"; private static final String ATTR_KEY = "key"; private static final String ATTR_VALUE_TYPE = "type"; private static final String ATTR_MULTIPLE = "m"; @@ -312,6 +314,14 @@ public class UserManagerService extends IUserManager.Stub { private long mLastRequestQuietModeEnabledMillis; + /** + * {@code true} if the system should ignore errors when preparing the + * storage directories for this user. This is {@code false} for all new + * users; it will only be {@code true} for users that already existed + * on-disk from an older version of Android. + */ + private boolean mIgnorePrepareStorageErrors; + void setLastRequestQuietModeEnabledMillis(long millis) { mLastRequestQuietModeEnabledMillis = millis; } @@ -320,6 +330,14 @@ public class UserManagerService extends IUserManager.Stub { return mLastRequestQuietModeEnabledMillis; } + boolean getIgnorePrepareStorageErrors() { + return mIgnorePrepareStorageErrors; + } + + void setIgnorePrepareStorageErrors() { + mIgnorePrepareStorageErrors = true; + } + void clearSeedAccountData() { seedAccountName = null; seedAccountType = null; @@ -3176,6 +3194,10 @@ public class UserManagerService extends IUserManager.Stub { serializer.endTag(/* namespace */ null, TAG_LAST_REQUEST_QUIET_MODE_ENABLED_CALL); } + serializer.startTag(/* namespace */ null, TAG_IGNORE_PREPARE_STORAGE_ERRORS); + serializer.text(String.valueOf(userData.getIgnorePrepareStorageErrors())); + serializer.endTag(/* namespace */ null, TAG_IGNORE_PREPARE_STORAGE_ERRORS); + serializer.endTag(null, TAG_USER); serializer.endDocument(); @@ -3285,6 +3307,7 @@ public class UserManagerService extends IUserManager.Stub { Bundle legacyLocalRestrictions = null; RestrictionsSet localRestrictions = null; Bundle globalRestrictions = null; + boolean ignorePrepareStorageErrors = true; // default is true for old users final TypedXmlPullParser parser = Xml.resolvePullParser(is); int type; @@ -3363,6 +3386,11 @@ public class UserManagerService extends IUserManager.Stub { if (type == XmlPullParser.TEXT) { lastRequestQuietModeEnabledTimestamp = Long.parseLong(parser.getText()); } + } else if (TAG_IGNORE_PREPARE_STORAGE_ERRORS.equals(tag)) { + type = parser.next(); + if (type == XmlPullParser.TEXT) { + ignorePrepareStorageErrors = Boolean.parseBoolean(parser.getText()); + } } } } @@ -3390,6 +3418,9 @@ public class UserManagerService extends IUserManager.Stub { userData.persistSeedData = persistSeedData; userData.seedAccountOptions = seedAccountOptions; userData.setLastRequestQuietModeEnabledMillis(lastRequestQuietModeEnabledTimestamp); + if (ignorePrepareStorageErrors) { + userData.setIgnorePrepareStorageErrors(); + } synchronized (mRestrictionsLock) { if (baseRestrictions != null) { @@ -5227,6 +5258,9 @@ public class UserManagerService extends IUserManager.Stub { pw.println(); } } + + pw.println(" Ignore errors preparing storage: " + + userData.getIgnorePrepareStorageErrors()); } } @@ -5716,6 +5750,14 @@ public class UserManagerService extends IUserManager.Stub { UserManagerService.this.setDefaultCrossProfileIntentFilters( profileUserId, userTypeDetails, restrictions, parentUserId); } + + @Override + public boolean shouldIgnorePrepareStorageErrors(int userId) { + synchronized (mUsersLock) { + UserData userData = mUsers.get(userId); + return userData != null && userData.getIgnorePrepareStorageErrors(); + } + } } /** From 7d4ab9b698a83f131585f25662ac9211302a3400 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Sat, 26 Mar 2022 01:08:07 +0000 Subject: [PATCH 5/5] Log to EventLog on prepareUserStorage failure Bug: 224585613 Change-Id: Id6dfb4f4c48d5cf4e71f54bdb6d0d6eea527caf5 Merged-In: Id6dfb4f4c48d5cf4e71f54bdb6d0d6eea527caf5 --- .../core/java/com/android/server/StorageManagerService.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/core/java/com/android/server/StorageManagerService.java b/services/core/java/com/android/server/StorageManagerService.java index 19596ba142cdb..0a23ab8184e5f 100644 --- a/services/core/java/com/android/server/StorageManagerService.java +++ b/services/core/java/com/android/server/StorageManagerService.java @@ -128,6 +128,7 @@ import android.util.ArrayMap; import android.util.ArraySet; import android.util.AtomicFile; import android.util.DataUnit; +import android.util.EventLog; import android.util.Log; import android.util.Pair; import android.util.Slog; @@ -3398,6 +3399,7 @@ class StorageManagerService extends IStorageManager.Stub } } } catch (Exception e) { + EventLog.writeEvent(0x534e4554, "224585613", -1, ""); Slog.wtf(TAG, e); // Make sure to re-throw this exception; we must not ignore failure // to prepare the user storage as it could indicate that encryption