From 8228658b691897f110c5d77195a97b2f2f2b2aeb Mon Sep 17 00:00:00 2001 From: Sergey Nikolaienkov Date: Fri, 8 Apr 2022 13:47:41 +0200 Subject: [PATCH] Guard against incorrect ID assignment in CdmService Do not assume that all previously assigned association IDs were assigned correctly in regard to ranges of associations ID allocated to each user when generating an association ID for a new association in CompanionDeviceManagerService.getNewAssociationIdForPackage(). Also Slog.e() when detect such errors in existing Associations as well as when detect ID conflicts in AssociationStoreImpl.addAssociation(). Bug: 224736262 Bug: 223995714 Test: atest CtsCompanionDeviceManagerCoreTestCases Change-Id: Ibe14cfda9718b3e473f0f157bbfef2f7051819c2 --- .../companion/AssociationStoreImpl.java | 3 ++- .../CompanionDeviceManagerService.java | 8 ++++++- .../server/companion/PersistentDataStore.java | 24 +++++++++++++++++-- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/services/companion/java/com/android/server/companion/AssociationStoreImpl.java b/services/companion/java/com/android/server/companion/AssociationStoreImpl.java index cb28254f70db6..bd2a97df1d7a6 100644 --- a/services/companion/java/com/android/server/companion/AssociationStoreImpl.java +++ b/services/companion/java/com/android/server/companion/AssociationStoreImpl.java @@ -23,6 +23,7 @@ import android.annotation.UserIdInt; import android.companion.AssociationInfo; import android.net.MacAddress; import android.util.Log; +import android.util.Slog; import android.util.SparseArray; import com.android.internal.annotations.GuardedBy; @@ -80,7 +81,7 @@ class AssociationStoreImpl implements AssociationStore { synchronized (mLock) { if (mIdMap.containsKey(id)) { - if (DEBUG) Log.w(TAG, "Association already stored."); + Slog.e(TAG, "Association with id " + id + " already exists."); return; } mIdMap.put(id, association); diff --git a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java index 13a5a28299458..02a0a95791233 100644 --- a/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java +++ b/services/companion/java/com/android/server/companion/CompanionDeviceManagerService.java @@ -813,7 +813,13 @@ public class CompanionDeviceManagerService extends SystemService { synchronized (mPreviouslyUsedIds) { // First: collect all IDs currently in use for this user's Associations. final SparseBooleanArray usedIds = new SparseBooleanArray(); - for (AssociationInfo it : mAssociationStore.getAssociationsForUser(userId)) { + + // We should really only be checking associations for the given user (i.e.: + // mAssociationStore.getAssociationsForUser(userId)), BUT in the past we've got in a + // state where association IDs were not assigned correctly in regard to + // user-to-association-ids-range (e.g. associations with IDs from 1 to 100,000 should + // always belong to u0), so let's check all the associations. + for (AssociationInfo it : mAssociationStore.getAssociations()) { usedIds.put(it.getId(), true); } diff --git a/services/companion/java/com/android/server/companion/PersistentDataStore.java b/services/companion/java/com/android/server/companion/PersistentDataStore.java index d0cc12286b128..36393894f7277 100644 --- a/services/companion/java/com/android/server/companion/PersistentDataStore.java +++ b/services/companion/java/com/android/server/companion/PersistentDataStore.java @@ -25,6 +25,8 @@ import static com.android.internal.util.XmlUtils.writeBooleanAttribute; import static com.android.internal.util.XmlUtils.writeIntAttribute; import static com.android.internal.util.XmlUtils.writeLongAttribute; import static com.android.internal.util.XmlUtils.writeStringAttribute; +import static com.android.server.companion.CompanionDeviceManagerService.getFirstAssociationIdForUser; +import static com.android.server.companion.CompanionDeviceManagerService.getLastAssociationIdForUser; import static com.android.server.companion.DataStoreUtils.createStorageFileForUser; import static com.android.server.companion.DataStoreUtils.isEndOfTag; import static com.android.server.companion.DataStoreUtils.isStartOfTag; @@ -194,7 +196,25 @@ final class PersistentDataStore { // Associations for all users are stored in a single "flat" set: so we read directly // into it. - readStateForUser(userId, allAssociationsOut, previouslyUsedIds); + final Set associationsForUser = new HashSet<>(); + readStateForUser(userId, associationsForUser, previouslyUsedIds); + + // Go through all the associations for the user and check if their IDs are within + // the allowed range (for the user). + final int firstAllowedId = getFirstAssociationIdForUser(userId); + final int lastAllowedId = getLastAssociationIdForUser(userId); + for (AssociationInfo association : associationsForUser) { + final int id = association.getId(); + if (id < firstAllowedId || id > lastAllowedId) { + Slog.e(TAG, "Wrong association ID assignment: " + id + ". " + + "Association belongs to u" + userId + " and thus its ID should be " + + "within [" + firstAllowedId + ", " + lastAllowedId + "] range."); + // TODO(b/224736262): try fixing (re-assigning) the ID? + } + } + + // Add user's association to the "output" set. + allAssociationsOut.addAll(associationsForUser); // Save previously used IDs for this user into the "out" structure. previouslyUsedIdsPerUserOut.append(userId, previouslyUsedIds); @@ -369,7 +389,7 @@ final class PersistentDataStore { // existing ones from the backup files. And the fact that we are reading from a V0 file, // means that CDM hasn't assigned any IDs yet, so we can just start from the first available // id for each user (eg. 1 for user 0; 100 001 - for user 1; 200 001 - for user 2; etc). - int associationId = CompanionDeviceManagerService.getFirstAssociationIdForUser(userId); + int associationId = getFirstAssociationIdForUser(userId); while (true) { parser.nextTag(); if (isEndOfTag(parser, XML_TAG_ASSOCIATIONS)) break;