From 27154a137212c88f7f7a3a4ad05c76ffb8cfddb3 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Fri, 30 Oct 2020 20:20:39 -0700 Subject: [PATCH] Fix more locking issues identified by @GuardedBy. - Add one missing locking for mRegistry.removePermission(). - Add @GuardedBy annotations for *Locked methods. - Remove unused mBackgroundPermissions. - Don't lock access to mOnPermissionChangeListeners because RemoteCallbackList has its own locking. - Lock access to mPrivappPermissionsViolations. Bug: 158736025 Test: presubmit Test: errorprone build logs/warnings.html Change-Id: I356b0d3100fa555b163e6c40dad78f4b516c521b --- .../permission/PermissionManagerService.java | 99 ++++++------------- 1 file changed, 32 insertions(+), 67 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java index 300206a679a28..81e8c2286755d 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerService.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerService.java @@ -270,13 +270,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { @GuardedBy("mLock") private PermissionPolicyInternal mPermissionPolicyInternal; - /** - * For each foreground/background permission the mapping: - * Background permission -> foreground permissions - */ - @GuardedBy("mLock") - private ArrayMap> mBackgroundPermissions; - /** * A permission backup might contain apps that are not installed. In this case we delay the * restoration until the app is installed. @@ -295,7 +288,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { @GuardedBy("mLock") private CheckPermissionDelegate mCheckPermissionDelegate; - @GuardedBy("mLock") + @NonNull private final OnPermissionChangeListeners mOnPermissionChangeListeners; @GuardedBy("mLock") @@ -959,6 +952,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return PackageManager.PERMISSION_DENIED; } + @GuardedBy("mLock") private boolean checkSinglePermissionInternalLocked(@NonNull UidPermissionState uidState, @NonNull String permissionName, boolean isInstantApp) { if (!uidState.isPermissionGranted(permissionName)) { @@ -1029,6 +1023,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return PackageManager.PERMISSION_DENIED; } + @GuardedBy("mLock") private boolean checkSingleUidPermissionInternalLocked(int uid, @NonNull String permissionName) { ArraySet permissions = mSystemPermissions.get(uid); @@ -1094,9 +1089,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { Manifest.permission.OBSERVE_GRANT_REVOKE_PERMISSIONS, "addOnPermissionsChangeListener"); - synchronized (mLock) { - mOnPermissionChangeListeners.addListenerLocked(listener); - } + mOnPermissionChangeListeners.addListener(listener); } @Override @@ -1104,9 +1097,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { if (mPackageManagerInt.getInstantAppPackageName(Binder.getCallingUid()) != null) { throw new SecurityException("Instant applications don't have access to this method"); } - synchronized (mLock) { - mOnPermissionChangeListeners.removeListenerLocked(listener); - } + mOnPermissionChangeListeners.removeListener(listener); } @Override @@ -3068,6 +3059,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { * * @return The updated value of the {@code updatedUserIds} parameter */ + @GuardedBy("mLock") private @NonNull int[] revokePermissionsNoLongerImplicitLocked(@NonNull UidPermissionState ps, @NonNull AndroidPackage pkg, int userId, @NonNull int[] updatedUserIds) { String pkgName = pkg.getPackageName(); @@ -3120,6 +3112,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { * @param ps The permission state of the package * @param pkg The package requesting the permissions */ + @GuardedBy("mLock") private void inheritPermissionStateToNewImplicitPermissionLocked( @NonNull ArraySet sourcePerms, @NonNull String newPerm, @NonNull UidPermissionState ps, @NonNull AndroidPackage pkg) { @@ -3191,6 +3184,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { * * @return List of users for which the permission state has been changed */ + @GuardedBy("mLock") private @NonNull int[] setInitialGrantForNewImplicitPermissionsLocked( @NonNull UidPermissionState origPs, @NonNull UidPermissionState ps, @NonNull AndroidPackage pkg, @NonNull ArraySet newImplicitPermissions, @@ -3582,11 +3576,13 @@ public class PermissionManagerService extends IPermissionManager.Stub { + packageName + " (" + pkg.getPath() + ") not in privapp-permissions whitelist"); if (RoSystemProperties.CONTROL_PRIVAPP_PERMISSIONS_ENFORCE) { - if (mPrivappPermissionsViolations == null) { - mPrivappPermissionsViolations = new ArraySet<>(); + synchronized (mLock) { + if (mPrivappPermissionsViolations == null) { + mPrivappPermissionsViolations = new ArraySet<>(); + } + mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): " + + permissionName); } - mPrivappPermissionsViolations.add(packageName + " (" + pkg.getPath() + "): " - + permissionName); } } } @@ -3952,6 +3948,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return affectedUserId; } + @GuardedBy("mLock") private boolean revokeUnusedSharedUserPermissionsLocked( List pkgList, UidPermissionState uidState) { // Collect all used permissions in the UID @@ -4043,35 +4040,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - /** - * Cache background->foreground permission mapping. - * - *

This is only run once. - */ - private void cacheBackgroundToForegoundPermissionMapping() { - synchronized (mLock) { - if (mBackgroundPermissions == null) { - // Cache background -> foreground permission mapping. - // Only system declares background permissions, hence mapping does never change. - mBackgroundPermissions = new ArrayMap<>(); - for (Permission bp : mRegistry.getPermissions()) { - if (bp.getBackgroundPermission() != null) { - String fgPerm = bp.getName(); - String bgPerm = bp.getBackgroundPermission(); - - List fgPerms = mBackgroundPermissions.get(bgPerm); - if (fgPerms == null) { - fgPerms = new ArrayList<>(); - mBackgroundPermissions.put(bgPerm, fgPerms); - } - - fgPerms.add(fgPerm); - } - } - } - } - } - /** * Update all packages on the volume, beside the changing package. If the changing * package is set too, all packages are updated. @@ -4145,8 +4113,6 @@ public class PermissionManagerService extends IPermissionManager.Stub { flags |= UPDATE_PERMISSIONS_ALL; } - cacheBackgroundToForegoundPermissionMapping(); - Trace.traceBegin(TRACE_TAG_PACKAGE_MANAGER, "restorePermissionState"); // Now update the permissions for all packages. if ((flags & UPDATE_PERMISSIONS_ALL) != 0) { @@ -4252,7 +4218,9 @@ public class PermissionManagerService extends IPermissionManager.Stub { } }); } - mRegistry.removePermission(bp.getName()); + synchronized (mLock) { + mRegistry.removePermission(bp.getName()); + } continue; } final AndroidPackage sourcePkg = @@ -4551,6 +4519,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return builder.toString(); } + @GuardedBy("mLock") private int calculateCurrentPermissionFootprintLocked(@NonNull Permission permissionTree) { int size = 0; for (final Permission permission : mRegistry.getPermissions()) { @@ -4559,6 +4528,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { return size; } + @GuardedBy("mLock") private void enforcePermissionCapLocked(PermissionInfo info, Permission tree) { // We calculate the max size of permissions defined by this uid and throw // if that plus the size of 'info' would exceed our stated maximum. @@ -4572,9 +4542,12 @@ public class PermissionManagerService extends IPermissionManager.Stub { private void systemReady() { mSystemReady = true; - if (mPrivappPermissionsViolations != null) { - throw new IllegalStateException("Signature|privileged permissions not in " - + "privapp-permissions whitelist: " + mPrivappPermissionsViolations); + + synchronized (mLock) { + if (mPrivappPermissionsViolations != null) { + throw new IllegalStateException("Signature|privileged permissions not in " + + "privapp-permissions whitelist: " + mPrivappPermissionsViolations); + } } mPermissionControllerManager = mContext.getSystemService(PermissionControllerManager.class); @@ -4642,29 +4615,21 @@ public class PermissionManagerService extends IPermissionManager.Stub { mMetricsLogger.write(log); } - /** - * Get the mapping of background permissions to their foreground permissions. - * - *

Only initialized in the system server. - * - * @return the map <bg permission -> list<fg perm>> - */ - public @Nullable ArrayMap> getBackgroundPermissions() { - return mBackgroundPermissions; - } - + @GuardedBy("mLock") @Nullable private UidPermissionState getUidStateLocked(@NonNull PackageSetting ps, @UserIdInt int userId) { return getUidStateLocked(ps.getAppId(), userId); } + @GuardedBy("mLock") @Nullable private UidPermissionState getUidStateLocked(@NonNull AndroidPackage pkg, @UserIdInt int userId) { return getUidStateLocked(pkg.getUid(), userId); } + @GuardedBy("mLock") @Nullable private UidPermissionState getUidStateLocked(@AppIdInt int appId, @UserIdInt int userId) { final UserPermissionState userState = mState.getUserState(userId); @@ -4707,6 +4672,7 @@ public class PermissionManagerService extends IPermissionManager.Stub { }); } + @GuardedBy("mLock") private void readLegacyPermissionStatesLocked(@NonNull UidPermissionState uidState, @NonNull Collection permissionStates) { for (final LegacyPermissionState.PermissionState permissionState : permissionStates) { @@ -5371,12 +5337,11 @@ public class PermissionManagerService extends IPermissionManager.Stub { } } - public void addListenerLocked(IOnPermissionsChangeListener listener) { + public void addListener(IOnPermissionsChangeListener listener) { mPermissionListeners.register(listener); - } - public void removeListenerLocked(IOnPermissionsChangeListener listener) { + public void removeListener(IOnPermissionsChangeListener listener) { mPermissionListeners.unregister(listener); }