From dde7ea23aa62c4fa4add29bdd85d20ecf4b7cd18 Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Tue, 1 Mar 2022 15:52:54 -0800 Subject: [PATCH 1/2] Properly fix revokePermissionsNoLongerImplicitLocked() for shared UIDs. This is actually a follow up to ag/13938162 that will properly fix the shared UID handling. If there are multiple packages in the same shared UID, the target SDK version of the UID should be the lowest target SDK version of those packages. This is also confirmed by existing code in PermissionManagerService and PackageManagerService. However, the current implicit permission revocation is only reading from the implicit permissions of the particular package, which is determined during package parsing according to the target SDK version of that package. So if a permission in one package in the shared UID is becoming explicit, it breaks the other package if it still relies on the permission being implicitly. And in reality, it's hard to make sure the other package has been updated to handle this situation. So in order to properly fix this, we need to collect all the implicit permissions in this shared UID and pass that to the revocation logic. This does mean we will be doing exactly the same thing again when we are handling other packages' permission state, but that won't be a big issue, whereas the alternative is to refactor the code to handle permission state by UID which is way too risky for T or backports. In the proposed new permission subsystem, we will indeed handle permissions by UID though. This change also fixes a potential deadlock bug introduce by PackageManagerService refactor - we should never call into other components while holding our mLock. Bug: 201263297 Test: presubmit Change-Id: I8315dc27b703d819e8970134d5cf17b0edbc22dc --- .../PermissionManagerServiceImpl.java | 70 ++++++++++--------- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java index 87494a62b6251..def0ed5680308 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java @@ -2536,33 +2536,38 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt } } + Collection uidRequestedPermissions; + Collection uidImplicitPermissions; + int uidTargetSdkVersion; + if (!ps.hasSharedUser()) { + uidRequestedPermissions = pkg.getRequestedPermissions(); + uidImplicitPermissions = pkg.getImplicitPermissions(); + uidTargetSdkVersion = pkg.getTargetSdkVersion(); + } else { + uidRequestedPermissions = new ArraySet<>(); + uidImplicitPermissions = new ArraySet<>(); + uidTargetSdkVersion = Build.VERSION_CODES.CUR_DEVELOPMENT; + final ArraySet packages = + mPackageManagerInt.getSharedUserPackages(ps.getSharedUserAppId()); + int packagesSize = packages.size(); + for (int i = 0; i < packagesSize; i++) { + AndroidPackageApi sharedUserPackage = + packages.valueAt(i).getAndroidPackage(); + uidRequestedPermissions.addAll( + sharedUserPackage.getRequestedPermissions()); + uidImplicitPermissions.addAll( + sharedUserPackage.getImplicitPermissions()); + uidTargetSdkVersion = Math.min(uidTargetSdkVersion, + sharedUserPackage.getTargetSdkVersion()); + } + } + synchronized (mLock) { for (final int userId : userIds) { final UserPermissionState userState = mState.getOrCreateUserState(userId); final UidPermissionState uidState = userState.getOrCreateUidState(ps.getAppId()); if (uidState.isMissing()) { - Collection uidRequestedPermissions; - int targetSdkVersion; - if (!ps.hasSharedUser()) { - uidRequestedPermissions = pkg.getRequestedPermissions(); - targetSdkVersion = pkg.getTargetSdkVersion(); - } else { - uidRequestedPermissions = new ArraySet<>(); - targetSdkVersion = Build.VERSION_CODES.CUR_DEVELOPMENT; - final ArraySet packages = - mPackageManagerInt.getSharedUserPackages(ps.getSharedUserAppId()); - int packagesSize = packages.size(); - for (int i = 0; i < packagesSize; i++) { - AndroidPackageApi sharedUserPackage = - packages.valueAt(i).getAndroidPackage(); - uidRequestedPermissions.addAll( - sharedUserPackage.getRequestedPermissions()); - targetSdkVersion = Math.min(targetSdkVersion, - sharedUserPackage.getTargetSdkVersion()); - } - } - for (String permissionName : uidRequestedPermissions) { Permission permission = mRegistry.getPermission(permissionName); if (permission == null) { @@ -2576,7 +2581,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt FLAG_PERMISSION_RESTRICTION_UPGRADE_EXEMPT, FLAG_PERMISSION_RESTRICTION_UPGRADE_EXEMPT); } - if (targetSdkVersion < Build.VERSION_CODES.M) { + if (uidTargetSdkVersion < Build.VERSION_CODES.M) { uidState.updatePermissionFlags(permission, PackageManager.FLAG_PERMISSION_REVIEW_REQUIRED | PackageManager.FLAG_PERMISSION_REVOKED_COMPAT, @@ -2909,8 +2914,9 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt userState.setInstallPermissionsFixed(ps.getPackageName(), true); } - updatedUserIds = revokePermissionsNoLongerImplicitLocked(uidState, pkg, - userId, updatedUserIds); + updatedUserIds = revokePermissionsNoLongerImplicitLocked(uidState, + pkg.getPackageName(), uidImplicitPermissions, uidTargetSdkVersion, userId, + updatedUserIds); updatedUserIds = setInitialGrantForNewImplicitPermissionsLocked(origState, uidState, pkg, newImplicitPermissions, userId, updatedUserIds); } @@ -2947,7 +2953,9 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt * {@link PackageManager#FLAG_PERMISSION_REVOKE_WHEN_REQUESTED} set. * * @param ps The state of the permissions of the package - * @param pkg The package that is currently looked at + * @param packageName The name of the package + * @param uidImplicitPermissions The implicit permissions of all packages in the UID + * @param uidTargetSdkVersion The lowest target SDK version of all packages in the UID * @param userIds All user IDs in the system, must be passed in because this method is locked * @param updatedUserIds a list of user ids that needs to be amended if the permission state * for a user is changed. @@ -2957,14 +2965,12 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt @NonNull @GuardedBy("mLock") private int[] revokePermissionsNoLongerImplicitLocked(@NonNull UidPermissionState ps, - @NonNull AndroidPackage pkg, int userId, @NonNull int[] updatedUserIds) { - String pkgName = pkg.getPackageName(); - boolean supportsRuntimePermissions = pkg.getTargetSdkVersion() - >= Build.VERSION_CODES.M; + @NonNull String packageName, @NonNull Collection uidImplicitPermissions, + int uidTargetSdkVersion, int userId, @NonNull int[] updatedUserIds) { + boolean supportsRuntimePermissions = uidTargetSdkVersion >= Build.VERSION_CODES.M; for (String permission : ps.getGrantedPermissions()) { - if (pkg.getRequestedPermissions().contains(permission) - && !pkg.getImplicitPermissions().contains(permission)) { + if (!uidImplicitPermissions.contains(permission)) { Permission bp = mRegistry.getPermission(permission); if (bp != null && bp.isRuntime()) { int flags = ps.getPermissionFlags(permission); @@ -2991,7 +2997,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt if (ps.revokePermission(bp)) { if (DEBUG_PERMISSIONS) { Slog.i(TAG, "Revoking runtime permission " - + permission + " for " + pkgName + + permission + " for " + packageName + " as it is now requested"); } } From 01221cb71625d2dc095e02a97554ca13b64c76ec Mon Sep 17 00:00:00 2001 From: Hai Zhang Date: Wed, 2 Mar 2022 02:10:15 +0000 Subject: [PATCH 2/2] Fix potential deadlock in restorePermissionState(). The call to retrieve all shared user packages was on the PackageSetting object and was lockless, however it was replaced with a PackageManagerInternal call in ag/16740473 and the method may take a lock. This creates a potential deadlock because we are already holding our own lock in some cases, and we shouldn't allow calling another system service while holding it. This change makes revokeUnusedSharedUserPermissionsLocked() re-use the uidRequestedPermissions that we calculated earlier, and the behavior of the method remains unchanged. Bug: 216207402 Test: presubmit Change-Id: I4b031cb670a6921bfa1425fff65f58cd28af576e --- .../PermissionManagerServiceImpl.java | 28 +++---------------- 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java index def0ed5680308..351c5be5e7910 100644 --- a/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java +++ b/services/core/java/com/android/server/pm/permission/PermissionManagerServiceImpl.java @@ -2611,8 +2611,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt // the runtime ones are written only if changed. The only cases of // changed runtime permissions here are promotion of an install to // runtime and revocation of a runtime from a shared user. - if (revokeUnusedSharedUserPermissionsLocked( - mPackageManagerInt.getSharedUserPackages(ps.getSharedUserAppId()), + if (revokeUnusedSharedUserPermissionsLocked(uidRequestedPermissions, uidState)) { updatedUserIds = ArrayUtils.appendInt(updatedUserIds, userId); runtimePermissionsRevoked = true; @@ -3828,27 +3827,8 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt @GuardedBy("mLock") private boolean revokeUnusedSharedUserPermissionsLocked( - ArraySet pkgList, UidPermissionState uidState) { - // Collect all used permissions in the UID - final ArraySet usedPermissions = new ArraySet<>(); - if (pkgList == null || pkgList.size() == 0) { - return false; - } - for (PackageStateInternal pkgState : pkgList) { - final AndroidPackageApi pkg = pkgState.getAndroidPackage(); - if (pkg.getRequestedPermissions().isEmpty()) { - continue; - } - final int requestedPermCount = pkg.getRequestedPermissions().size(); - for (int j = 0; j < requestedPermCount; j++) { - String permission = pkg.getRequestedPermissions().get(j); - Permission bp = mRegistry.getPermission(permission); - if (bp != null) { - usedPermissions.add(permission); - } - } - } - + @NonNull Collection uidRequestedPermissions, + @NonNull UidPermissionState uidState) { boolean runtimePermissionChanged = false; // Prune permissions @@ -3856,7 +3836,7 @@ public class PermissionManagerServiceImpl implements PermissionManagerServiceInt final int permissionStatesSize = permissionStates.size(); for (int i = permissionStatesSize - 1; i >= 0; i--) { PermissionState permissionState = permissionStates.get(i); - if (!usedPermissions.contains(permissionState.getName())) { + if (!uidRequestedPermissions.contains(permissionState.getName())) { Permission bp = mRegistry.getPermission(permissionState.getName()); if (bp != null) { if (uidState.removePermissionState(bp.getName()) && bp.isRuntime()) {