From 6720f179f877ca06527e4c1eff04fff2e10ac824 Mon Sep 17 00:00:00 2001 From: Tianjie Date: Sat, 17 Apr 2021 19:42:26 -0700 Subject: [PATCH 1/2] Check the return value of RoR preparation/clear The corresponding functions already has a boolean return value in the reboot escrow manager. So expose the return value to the recovery system. Test: build, check return value of request Change-Id: I056956075868ab62ba6a43259242c4521c1c1bfe --- .../internal/widget/LockSettingsInternal.java | 4 ++-- .../locksettings/LockSettingsService.java | 10 +++++---- .../recoverysystem/RecoverySystemService.java | 21 ++++++++++++------- .../RecoverySystemServiceTest.java | 11 +++++++++- 4 files changed, 31 insertions(+), 15 deletions(-) diff --git a/core/java/com/android/internal/widget/LockSettingsInternal.java b/core/java/com/android/internal/widget/LockSettingsInternal.java index 940979d7cd1f8..0a2c18f83fef3 100644 --- a/core/java/com/android/internal/widget/LockSettingsInternal.java +++ b/core/java/com/android/internal/widget/LockSettingsInternal.java @@ -110,7 +110,7 @@ public abstract class LockSettingsInternal { * #setRebootEscrowListener}, then {@link #armRebootEscrow()} should be called before * rebooting to apply the update. */ - public abstract void prepareRebootEscrow(); + public abstract boolean prepareRebootEscrow(); /** * Registers a listener for when the RebootEscrow HAL has stored its data needed for rebooting @@ -124,7 +124,7 @@ public abstract class LockSettingsInternal { /** * Requests that any data needed for rebooting is cleared from the RebootEscrow HAL. */ - public abstract void clearRebootEscrow(); + public abstract boolean clearRebootEscrow(); /** * Should be called immediately before rebooting for an update. This depends on {@link diff --git a/services/core/java/com/android/server/locksettings/LockSettingsService.java b/services/core/java/com/android/server/locksettings/LockSettingsService.java index ca5f7b3869b29..117c85bfdf774 100644 --- a/services/core/java/com/android/server/locksettings/LockSettingsService.java +++ b/services/core/java/com/android/server/locksettings/LockSettingsService.java @@ -3467,11 +3467,12 @@ public class LockSettingsService extends ILockSettings.Stub { } @Override - public void prepareRebootEscrow() { + public boolean prepareRebootEscrow() { if (!mRebootEscrowManager.prepareRebootEscrow()) { - return; + return false; } mStrongAuth.requireStrongAuth(STRONG_AUTH_REQUIRED_FOR_UNATTENDED_UPDATE, USER_ALL); + return true; } @Override @@ -3480,12 +3481,13 @@ public class LockSettingsService extends ILockSettings.Stub { } @Override - public void clearRebootEscrow() { + public boolean clearRebootEscrow() { if (!mRebootEscrowManager.clearRebootEscrow()) { - return; + return false; } mStrongAuth.noLongerRequireStrongAuth(STRONG_AUTH_REQUIRED_FOR_UNATTENDED_UPDATE, USER_ALL); + return true; } @Override diff --git a/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java b/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java index 81a51e2906641..24337f3ad346f 100644 --- a/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java +++ b/services/core/java/com/android/server/recoverysystem/RecoverySystemService.java @@ -554,11 +554,15 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo case ROR_NEED_PREPARATION: final long origId = Binder.clearCallingIdentity(); try { - mInjector.getLockSettingsService().prepareRebootEscrow(); + boolean result = mInjector.getLockSettingsService().prepareRebootEscrow(); + // Clear the RoR preparation state if lock settings reports an failure. + if (!result) { + clearRoRPreparationState(); + } + return result; } finally { Binder.restoreCallingIdentity(origId); } - return true; default: throw new IllegalStateException("Unsupported action type on new request " + action); } @@ -670,11 +674,10 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo case ROR_REQUESTED_NEED_CLEAR: final long origId = Binder.clearCallingIdentity(); try { - mInjector.getLockSettingsService().clearRebootEscrow(); + return mInjector.getLockSettingsService().clearRebootEscrow(); } finally { Binder.restoreCallingIdentity(origId); } - return true; default: throw new IllegalStateException("Unsupported action type on clear " + action); } @@ -820,6 +823,11 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo lskfCapturedCount); } + private synchronized void clearRoRPreparationState() { + mCallerPendingRequest.clear(); + mCallerPreparedForReboot.clear(); + } + private void clearRoRPreparationStateOnRebootFailure(RebootPreparationError escrowError) { if (!FATAL_ARM_ESCROW_ERRORS.contains(escrowError.mProviderErrorCode)) { return; @@ -827,10 +835,7 @@ public class RecoverySystemService extends IRecoverySystem.Stub implements Reboo Slog.w(TAG, "Clearing resume on reboot states for all clients on arm escrow error: " + escrowError.mProviderErrorCode); - synchronized (this) { - mCallerPendingRequest.clear(); - mCallerPreparedForReboot.clear(); - } + clearRoRPreparationState(); } private @ResumeOnRebootRebootErrorCode int rebootWithLskfImpl(String packageName, String reason, diff --git a/services/tests/servicestests/src/com/android/server/recoverysystem/RecoverySystemServiceTest.java b/services/tests/servicestests/src/com/android/server/recoverysystem/RecoverySystemServiceTest.java index b64810b665421..5234bb725a8f6 100644 --- a/services/tests/servicestests/src/com/android/server/recoverysystem/RecoverySystemServiceTest.java +++ b/services/tests/servicestests/src/com/android/server/recoverysystem/RecoverySystemServiceTest.java @@ -95,6 +95,8 @@ public class RecoverySystemServiceTest { mUncryptUpdateFileWriter = mock(FileWriter.class); mLockSettingsInternal = mock(LockSettingsInternal.class); + doReturn(true).when(mLockSettingsInternal).prepareRebootEscrow(); + doReturn(true).when(mLockSettingsInternal).clearRebootEscrow(); doReturn(LockSettingsInternal.ARM_REBOOT_ERROR_NONE).when(mLockSettingsInternal) .armRebootEscrow(); @@ -254,7 +256,6 @@ public class RecoverySystemServiceTest { + RecoverySystemService.REQUEST_LSKF_TIMESTAMP_PREF_SUFFIX), eq(100_000L)); } - @Test public void requestLskf_success() throws Exception { IntentSender intentSender = mock(IntentSender.class); @@ -298,6 +299,14 @@ public class RecoverySystemServiceTest { anyInt(), anyInt(), anyInt()); } + @Test + public void requestLskf_lockSettingsError() throws Exception { + IntentSender intentSender = mock(IntentSender.class); + + doReturn(false).when(mLockSettingsInternal).prepareRebootEscrow(); + assertFalse(mRecoverySystemService.requestLskf(FAKE_OTA_PACKAGE_NAME, intentSender)); + } + @Test public void isLskfCaptured_requestedButNotPrepared() throws Exception { IntentSender intentSender = mock(IntentSender.class); From ec75e51b478d7ff407b49a0296c4c8677da05c12 Mon Sep 17 00:00:00 2001 From: Tianjie Date: Tue, 20 Apr 2021 22:23:38 -0700 Subject: [PATCH 2/2] Add RecoverySystemServiceTest to presubmit Test: TH Change-Id: I296c10117cbd0b73bf265793667769851c0e99ec --- .../android/server/recoverysystem/TEST_MAPPING | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 services/tests/servicestests/src/com/android/server/recoverysystem/TEST_MAPPING diff --git a/services/tests/servicestests/src/com/android/server/recoverysystem/TEST_MAPPING b/services/tests/servicestests/src/com/android/server/recoverysystem/TEST_MAPPING new file mode 100644 index 0000000000000..4d1e4051032f6 --- /dev/null +++ b/services/tests/servicestests/src/com/android/server/recoverysystem/TEST_MAPPING @@ -0,0 +1,15 @@ +{ + "presubmit": [ + { + "name": "FrameworksServicesTests", + "options": [ + { + "include-filter": "com.android.server.recoverysystem." + }, + { + "exclude-annotation": "android.platform.test.annotations.FlakyTest" + } + ] + } + ] +} \ No newline at end of file