From 3ce0ee5363c9325e5c95dadbe24a03d94aedc90e Mon Sep 17 00:00:00 2001 From: Jooyung Han Date: Thu, 23 Jan 2020 13:23:26 +0900 Subject: [PATCH 01/26] Use optional for nullable types AIDL generates optional for nullable T types for C++, which is more efficient and idomatic and easy to use. Bug: 144773267 Test: build/flash/boot Change-Id: I98549c8614c9152d5d45e2f1f33f2f3c31a9bbbf --- VoldNativeService.cpp | 4 ++-- VoldNativeService.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 67bc939..06b141f 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -800,7 +800,7 @@ binder::Status VoldNativeService::lockUserKey(int32_t userId) { return translateBool(fscrypt_lock_user_key(userId)); } -binder::Status VoldNativeService::prepareUserStorage(const std::unique_ptr& uuid, +binder::Status VoldNativeService::prepareUserStorage(const std::optional& uuid, int32_t userId, int32_t userSerial, int32_t flags) { ENFORCE_SYSTEM_OR_ROOT; @@ -812,7 +812,7 @@ binder::Status VoldNativeService::prepareUserStorage(const std::unique_ptr& uuid, +binder::Status VoldNativeService::destroyUserStorage(const std::optional& uuid, int32_t userId, int32_t flags) { ENFORCE_SYSTEM_OR_ROOT; std::string empty_string = ""; diff --git a/VoldNativeService.h b/VoldNativeService.h index 7de2a67..cbb0a80 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -122,9 +122,9 @@ class VoldNativeService : public BinderService, public os::Bn const std::string& secret); binder::Status lockUserKey(int32_t userId); - binder::Status prepareUserStorage(const std::unique_ptr& uuid, int32_t userId, + binder::Status prepareUserStorage(const std::optional& uuid, int32_t userId, int32_t userSerial, int32_t flags); - binder::Status destroyUserStorage(const std::unique_ptr& uuid, int32_t userId, + binder::Status destroyUserStorage(const std::optional& uuid, int32_t userId, int32_t flags); binder::Status prepareSandboxForApp(const std::string& packageName, int32_t appId, From d88e090098d4a95112aecb135d1bcba96150bdd1 Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Thu, 20 Feb 2020 16:10:01 +0000 Subject: [PATCH 02/26] Add Android/data mounting along with obb mounting in vold We should mount Android/data also, not only Android/obb. Test: After flag is enabled, AdoptableHostTest still pass. Bug: 148049767 Change-Id: I26dc3756aa5843b85565495e9c2698130113f49a --- VolumeManager.cpp | 75 ++++++++++++++++++++++++++++++---------- VolumeManager.h | 2 +- model/EmulatedVolume.cpp | 4 +-- 3 files changed, 59 insertions(+), 22 deletions(-) diff --git a/VolumeManager.cpp b/VolumeManager.cpp index 9efe01a..8228e59 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -738,7 +738,7 @@ int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { forkAndRemountChild, &mountMode) ? 0 : -1; } -// Bind mount obb dir for an app if necessary. +// Bind mount obb & data dir for an app if necessary. // How it works: // 1). Check if a pid is an app uid and not the FuseDaemon, if not then return. // 2). Get the mounts for that pid. @@ -746,18 +746,18 @@ int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { // 4). Get all packages and uid mounted for jit profile. These packages are all packages with // same uid or whitelisted apps. // 5a). If there's no package, it means it's not a process running app data isolation, so -// just bind mount Android/obb dir. +// just bind mount Android/obb & Android/data dir. // 5b). Otherwise, for each package, create obb dir if it's not created and bind mount it. // TODO: Should we get some reliable data from system server instead of scanning /proc ? -static bool bindMountAppObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, void* params) { +static bool bindMountAppDataObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, void* params) { if (uid < AID_APP_START || uid > AID_APP_END) { return true; } if (android::vold::IsFuseDaemon(pid)) { return true; } - async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Start mounting obb for uid:%d, pid:%d", uid, - pid); + async_safe_format_log(ANDROID_LOG_ERROR, "vold", + "Start mounting obb and data for uid:%d, pid:%d", uid, pid); userid_t userId = multiuser_get_user_id(uid); if (setns(nsFd, CLONE_NEWNS) != 0) { @@ -782,6 +782,7 @@ static bool bindMountAppObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, } // Check if obb directory is mounted, and get all packages of mounted app data directory. + // We only need to check obb directory and assume if obb is mounted, data is mounted also. bool obb_mounted = false; std::vector pkg_name_list; mntent* mentry; @@ -799,47 +800,70 @@ static bool bindMountAppObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, return true; } - // Ensure obb parent directory exists - std::string obbSource; + std::string obbSource, dataSource; if (IsFilesystemSupported("sdcardfs")) { obbSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/obb", userId); + dataSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/data", userId); } else { obbSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/obb", userId, userId); + dataSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/data", userId, userId); } std::string obbTarget(StringPrintf("/storage/emulated/%d/Android/obb", userId)); + std::string dataTarget(StringPrintf("/storage/emulated/%d/Android/data", userId)); + + // TODO: Review if these checks are still necessary auto status = EnsureDirExists(obbSource, 0771, AID_MEDIA_RW, AID_MEDIA_RW); if (status != OK) { async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to create dir %s %s", obbSource.c_str(), strerror(-status)); return false; } + status = EnsureDirExists(dataSource, 0771, AID_MEDIA_RW, AID_MEDIA_RW); + if (status != OK) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to create dir %s %s", + dataSource.c_str(), strerror(-status)); + return false; + } // It means app data isolation is not applied to this, so we can just bind the whole obb // directory instead. if (pkg_name_list.empty()) { async_safe_format_log(ANDROID_LOG_INFO, "vold", - "Bind mounting whole obb directory for pid %d", pid); - status = BindMount(obbSource, obbTarget); - if (status != OK) { + "Bind mounting whole obb and data directory for pid %d", pid); + auto status1 = BindMount(obbSource, obbTarget); + // Still bind mount data even obb fails, just slower to access obb dir + auto status2 = BindMount(dataSource, dataTarget); + if (status1 != OK) { async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s", obbSource.c_str(), obbTarget.c_str(), strerror(-status)); return false; } + if (status2 != OK) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s", + dataSource.c_str(), dataTarget.c_str(), strerror(-status)); + return false; + } return true; } // Bind mount each app's obb directory for (const auto& pkg_name : pkg_name_list) { - std::string appObbSource; + std::string appObbSource, appDataSource; if (IsFilesystemSupported("sdcardfs")) { appObbSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/obb/%s", userId, pkg_name.c_str()); + appDataSource = StringPrintf("/mnt/runtime/default/emulated/%d/Android/data/%s", + userId, pkg_name.c_str()); } else { appObbSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/obb/%s", userId, userId, pkg_name.c_str()); + appDataSource = StringPrintf("/mnt/pass_through/%d/emulated/%d/Android/data/%s", + userId, userId, pkg_name.c_str()); } std::string appObbTarget(StringPrintf("/storage/emulated/%d/Android/obb/%s", userId, pkg_name.c_str())); + std::string appDataTarget(StringPrintf("/storage/emulated/%d/Android/data/%s", + userId, pkg_name.c_str())); status = EnsureDirExists(appObbSource, 0770, uid, AID_MEDIA_RW); if (status != OK) { @@ -847,24 +871,37 @@ static bool bindMountAppObbDir(uid_t uid, pid_t pid, int nsFd, const char* name, appObbSource.c_str()); continue; } - async_safe_format_log(ANDROID_LOG_INFO, "vold", - "Bind mounting app obb directory(%s) for pid %d", pkg_name.c_str(), - pid); - status = BindMount(appObbSource, appObbTarget); + status = EnsureDirExists(appDataSource, 0770, uid, AID_MEDIA_RW); if (status != OK) { + async_safe_format_log(ANDROID_LOG_INFO, "vold", "Failed to ensure dir %s exists", + appDataSource.c_str()); + continue; + } + async_safe_format_log(ANDROID_LOG_INFO, "vold", + "Bind mounting app obb and data directory(%s) for pid %d", + pkg_name.c_str(), pid); + auto status1 = BindMount(appObbSource, appObbTarget); + // Still bind mount data even obb fails, just slower to access obb dir + auto status2 = BindMount(appDataSource, appDataTarget); + if (status1 != OK) { async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s", obbSource.c_str(), obbTarget.c_str(), strerror(-status)); continue; } + if (status2 != OK) { + async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Failed to mount %s %s %s", + appDataSource.c_str(), appDataTarget.c_str(), strerror(-status)); + continue; + } } return true; } -int VolumeManager::remountAppObb(userid_t userId) { +int VolumeManager::remountAppStorageDirs(userid_t userId) { if (!GetBoolProperty(android::vold::kPropFuse, false)) { return 0; } - LOG(INFO) << "Start remounting app obb"; + LOG(INFO) << "Start remounting app obb and data"; pid_t child; if (!(child = fork())) { // Child process @@ -872,12 +909,12 @@ int VolumeManager::remountAppObb(userid_t userId) { PLOG(FATAL) << "Cannot create daemon"; } // TODO(149548518): Refactor the code so minimize the work after fork to prevent deadlock. - if (scanProcProcesses(0, userId, bindMountAppObbDir, nullptr)) { + if (scanProcProcesses(0, userId, bindMountAppDataObbDir, nullptr)) { // As some forked zygote processes may not setuid and recognized as an app yet, sleep // 3s and try again to catch 'em all. usleep(3 * 1000 * 1000); // 3s async_safe_format_log(ANDROID_LOG_ERROR, "vold", "Retry remounting app obb"); - scanProcProcesses(0, userId, bindMountAppObbDir, nullptr); + scanProcProcesses(0, userId, bindMountAppDataObbDir, nullptr); _exit(0); } else { _exit(1); diff --git a/VolumeManager.h b/VolumeManager.h index 479d99f..ef57040 100644 --- a/VolumeManager.h +++ b/VolumeManager.h @@ -118,7 +118,7 @@ class VolumeManager { int setPrimary(const std::shared_ptr& vol); int remountUid(uid_t uid, int32_t remountMode); - int remountAppObb(userid_t userId); + int remountAppStorageDirs(userid_t userId); bool addFuseMountedUser(userid_t userId); bool removeFuseMountedUser(userid_t userId); diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index c2f92e4..1391685 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -127,7 +127,7 @@ status_t EmulatedVolume::mountFuseBindMounts() { } if (mAppDataIsolationEnabled) { - // Starting from now, fuse is running, and zygote will bind app obb data directory + // Starting from now, fuse is running, and zygote will bind app obb & data directory if (!VolumeManager::Instance()->addFuseMountedUser(userId)) { return UNKNOWN_ERROR; } @@ -135,7 +135,7 @@ status_t EmulatedVolume::mountFuseBindMounts() { // As all new processes created by zygote will bind app obb data directory, we just need // to have a snapshot of all existing processes and see if any existing process needs to // remount obb data directory. - VolumeManager::Instance()->remountAppObb(userId); + VolumeManager::Instance()->remountAppStorageDirs(userId); } return OK; From f8cee5458ede8c6471efdec2c23c6abfdfa56fc0 Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Mon, 29 Jun 2020 16:07:48 +0100 Subject: [PATCH 03/26] Change mounting storage data and obb to on by default Bug: 151316657 Test: atest AdoptableHostTest Test: pass cts/cts_postsubmit_cf_stable-cloud-tf Change-Id: I2e500209b22c81a1060182210c507a52ec43a768 --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 26d9582..8e6ac1b 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::~EmulatedVolume() {} From 415d99dd2088c405fce9971f10082fbfd2e47261 Mon Sep 17 00:00:00 2001 From: Zim Date: Mon, 29 Jun 2020 19:50:47 +0100 Subject: [PATCH 04/26] Remove persist.sys.fuse == false code paths Since Android R, the FUSE prop is always on and FUSE-off is no longer supported Test: m Bug: 160159282 Change-Id: Ic4414b850511fe3b4fc6df3f8b736d21335db820 --- Utils.h | 1 - VoldNativeService.cpp | 6 ----- VolumeManager.cpp | 48 --------------------------------- VolumeManager.h | 4 +-- model/EmulatedVolume.cpp | 4 +-- model/PublicVolume.cpp | 57 +++++++++++++++++++--------------------- 6 files changed, 29 insertions(+), 91 deletions(-) diff --git a/Utils.h b/Utils.h index 04cbac4..0def28b 100644 --- a/Utils.h +++ b/Utils.h @@ -34,7 +34,6 @@ struct DIR; namespace android { namespace vold { -static const char* kPropFuse = "persist.sys.fuse"; static const char* kVoldAppDataIsolationEnabled = "persist.sys.vold_app_data_isolation_enabled"; static const char* kExternalStorageSdcardfs = "external_storage.sdcardfs.enabled"; diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 241c2a0..2d782cf 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -282,12 +282,6 @@ binder::Status VoldNativeService::mount( return translate(res); } - if ((mountFlags & MOUNT_FLAG_PRIMARY) != 0) { - res = VolumeManager::Instance()->setPrimary(vol); - if (res != OK) { - return translate(res); - } - } return translate(OK); } diff --git a/VolumeManager.cpp b/VolumeManager.cpp index a543573..8319b24 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -379,21 +379,6 @@ int VolumeManager::forgetPartition(const std::string& partGuid, const std::strin return success ? 0 : -1; } -int VolumeManager::linkPrimary(userid_t userId) { - if (!GetBoolProperty(android::vold::kPropFuse, false)) { - std::string source(mPrimary->getPath()); - if (mPrimary->isEmulated()) { - source = StringPrintf("%s/%d", source.c_str(), userId); - fs_prepare_dir(source.c_str(), 0755, AID_ROOT, AID_ROOT); - } - - std::string target(StringPrintf("/mnt/user/%d/primary", userId)); - LOG(DEBUG) << "Linking " << source << " to " << target; - Symlink(source, target); - } - return 0; -} - void VolumeManager::destroyEmulatedVolumesForUser(userid_t userId) { // Destroy and remove all unstacked EmulatedVolumes for the user auto i = mInternalEmulatedVolumes.begin(); @@ -474,18 +459,6 @@ int VolumeManager::onUserStarted(userid_t userId) { createEmulatedVolumesForUser(userId); } - if (!GetBoolProperty(android::vold::kPropFuse, false)) { - // Note that sometimes the system will spin up processes from Zygote - // before actually starting the user, so we're okay if Zygote - // already created this directory. - std::string path(StringPrintf("%s/%d", kPathUserMount, userId)); - fs_prepare_dir(path.c_str(), 0755, AID_ROOT, AID_ROOT); - - if (mPrimary) { - linkPrimary(userId); - } - } - mStartedUsers.insert(userId); createPendingDisksIfNeeded(); @@ -522,14 +495,6 @@ int VolumeManager::onSecureKeyguardStateChanged(bool isShowing) { return 0; } -int VolumeManager::setPrimary(const std::shared_ptr& vol) { - mPrimary = vol; - for (userid_t userId : mStartedUsers) { - linkPrimary(userId); - } - return 0; -} - // This code is executed after a fork so it's very important that the set of // methods we call here is strictly limited. // @@ -728,16 +693,6 @@ bool scanProcProcesses(uid_t uid, userid_t userId, ScanProcCallback callback, vo return true; } -int VolumeManager::remountUid(uid_t uid, int32_t mountMode) { - if (GetBoolProperty(android::vold::kPropFuse, false)) { - // TODO(135341433): Implement fuse specific logic. - return 0; - } - return scanProcProcesses(uid, static_cast(-1), - forkAndRemountChild, &mountMode) ? 0 : -1; -} - - // In each app's namespace, mount tmpfs on obb and data dir, and bind mount obb and data // package dirs. static bool remountStorageDirs(int nsFd, const char* android_data_dir, const char* android_obb_dir, @@ -884,9 +839,6 @@ bool VolumeManager::forkAndRemountStorage(int uid, int pid, int VolumeManager::remountAppStorageDirs(int uid, int pid, const std::vector& packageNames) { - if (!GetBoolProperty(android::vold::kPropFuse, false)) { - return 0; - } // Only run the remount if fuse is mounted for that user. userid_t userId = multiuser_get_user_id(uid); bool fuseMounted = false; diff --git a/VolumeManager.h b/VolumeManager.h index 3277f75..23a7fe0 100644 --- a/VolumeManager.h +++ b/VolumeManager.h @@ -115,9 +115,7 @@ class VolumeManager { void createPendingDisksIfNeeded(); int onSecureKeyguardStateChanged(bool isShowing); - int setPrimary(const std::shared_ptr& vol); - - int remountUid(uid_t uid, int32_t remountMode); + int remountUid(uid_t uid, int32_t remountMode) { return 0; } int remountAppStorageDirs(int uid, int pid, const std::vector& packageNames); /* Aborts all FUSE filesystems, in case the FUSE daemon is no longer up. */ diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index b2e1cd9..7c96ea2 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -301,8 +301,6 @@ status_t EmulatedVolume::doMount() { dev_t before = GetDevice(mSdcardFsFull); - bool isFuse = base::GetBoolProperty(kPropFuse, false); - // Mount sdcardfs regardless of FUSE, since we need it to bind-mount on top of the // FUSE volume for various reasons. if (mUseSdcardFs && getMountUserId() == 0) { @@ -350,7 +348,7 @@ status_t EmulatedVolume::doMount() { sdcardFsPid = 0; } - if (isFuse && isVisible) { + if (isVisible) { // Make sure we unmount sdcardfs if we bail out with an error below auto sdcardfs_unmounter = [&]() { LOG(INFO) << "sdcardfs_unmounter scope_guard running"; diff --git a/model/PublicVolume.cpp b/model/PublicVolume.cpp index d40e3e3..12e31ff 100644 --- a/model/PublicVolume.cpp +++ b/model/PublicVolume.cpp @@ -227,39 +227,36 @@ status_t PublicVolume::doMount() { TEMP_FAILURE_RETRY(waitpid(sdcardFsPid, nullptr, 0)); } - bool isFuse = base::GetBoolProperty(kPropFuse, false); - if (isFuse) { - // We need to mount FUSE *after* sdcardfs, since the FUSE daemon may depend - // on sdcardfs being up. - LOG(INFO) << "Mounting public fuse volume"; - android::base::unique_fd fd; - int user_id = getMountUserId(); - int result = MountUserFuse(user_id, getInternalPath(), stableName, &fd); + // We need to mount FUSE *after* sdcardfs, since the FUSE daemon may depend + // on sdcardfs being up. + LOG(INFO) << "Mounting public fuse volume"; + android::base::unique_fd fd; + int user_id = getMountUserId(); + int result = MountUserFuse(user_id, getInternalPath(), stableName, &fd); - if (result != 0) { - LOG(ERROR) << "Failed to mount public fuse volume"; - doUnmount(); - return -result; - } - - mFuseMounted = true; - auto callback = getMountCallback(); - if (callback) { - bool is_ready = false; - callback->onVolumeChecking(std::move(fd), getPath(), getInternalPath(), &is_ready); - if (!is_ready) { - LOG(ERROR) << "Failed to complete public volume mount"; - doUnmount(); - return -EIO; - } - } - - ConfigureReadAheadForFuse(GetFuseMountPathForUser(user_id, stableName), 256u); - - // See comment in model/EmulatedVolume.cpp - ConfigureMaxDirtyRatioForFuse(GetFuseMountPathForUser(user_id, stableName), 40u); + if (result != 0) { + LOG(ERROR) << "Failed to mount public fuse volume"; + doUnmount(); + return -result; } + mFuseMounted = true; + auto callback = getMountCallback(); + if (callback) { + bool is_ready = false; + callback->onVolumeChecking(std::move(fd), getPath(), getInternalPath(), &is_ready); + if (!is_ready) { + LOG(ERROR) << "Failed to complete public volume mount"; + doUnmount(); + return -EIO; + } + } + + ConfigureReadAheadForFuse(GetFuseMountPathForUser(user_id, stableName), 256u); + + // See comment in model/EmulatedVolume.cpp + ConfigureMaxDirtyRatioForFuse(GetFuseMountPathForUser(user_id, stableName), 40u); + return OK; } From d402fd8e082d76462d462be0d50edf0e8a38d101 Mon Sep 17 00:00:00 2001 From: Adam Wright Date: Wed, 8 Jul 2020 09:21:25 +0000 Subject: [PATCH 05/26] Revert "Change mounting storage data and obb to on by default" Revert submission 10829484-test_flag_change Reason for revert: Trialing a revert for b/160307959, b/160604224 based on bisection research. This won't land without discussion. Reverted Changes: Ifbf604716:Change mounting storage data and obb to on by defa... I2e500209b:Change mounting storage data and obb to on by defa... Change-Id: I3ff98f225243a759549cbf03d113d662e36e3fbe --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 8e6ac1b..26d9582 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::~EmulatedVolume() {} From 6c6b180e24988b094834693d47fba2adffdb120f Mon Sep 17 00:00:00 2001 From: Corina Date: Wed, 5 Aug 2020 11:07:02 +0100 Subject: [PATCH 06/26] Add core scoped storage host test to test mapping. Bug: 161990628 Test: atest CtsScopedStorageCoreHostTest Change-Id: I3a02f1dd4a280c04f305792696e800c378bcd9a8 --- TEST_MAPPING | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TEST_MAPPING b/TEST_MAPPING index 4f62642..15a4af4 100644 --- a/TEST_MAPPING +++ b/TEST_MAPPING @@ -1,5 +1,8 @@ { "presubmit": [ + { + "name": "CtsScopedStorageCoreHostTest" + }, { "name": "CtsScopedStorageHostTest" }, From 3bb866429b0a7ac51a5dd83ce32951a897678bbb Mon Sep 17 00:00:00 2001 From: Daniel Rosenberg Date: Wed, 12 Aug 2020 18:31:43 -0700 Subject: [PATCH 07/26] Set media folder +F for adopted storage as well We previously only set +F for /data/media, but adopted storage needs this as well. Instead we add support for adding attrs to PrepareDir. Bug: 163453310 Test: sm set-virtual-disk true follow UI setup and confirm +F on /mnt/expand/*/media Change-Id: I08f13b57a4de3538e88b38eb95b0ac115a5a5ce8 --- Utils.cpp | 30 +++++++++++++++++++++++++++++- Utils.h | 3 ++- model/PrivateVolume.cpp | 5 ++++- 3 files changed, 35 insertions(+), 3 deletions(-) diff --git a/Utils.cpp b/Utils.cpp index a9b7440..17921e8 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -416,7 +416,32 @@ int PrepareAppDirFromRoot(const std::string& path, const std::string& root, int return OK; } -status_t PrepareDir(const std::string& path, mode_t mode, uid_t uid, gid_t gid) { +int SetAttrs(const std::string& path, unsigned int attrs) { + unsigned long flags; + android::base::unique_fd fd( + TEMP_FAILURE_RETRY(open(path.c_str(), O_RDONLY | O_NONBLOCK | O_CLOEXEC))); + + if (fd == -1) { + PLOG(ERROR) << "Failed to open " << path; + return -1; + } + + if (ioctl(fd, FS_IOC_GETFLAGS, (void*)&flags)) { + PLOG(ERROR) << "Failed to get flags for " << path; + return -1; + } + + if ((flags & attrs) == attrs) return 0; + flags |= attrs; + if (ioctl(fd, FS_IOC_SETFLAGS, (void*)&flags)) { + PLOG(ERROR) << "Failed to set flags for " << path << "(0x" << std::hex << attrs << ")"; + return -1; + } + return 0; +} + +status_t PrepareDir(const std::string& path, mode_t mode, uid_t uid, gid_t gid, + unsigned int attrs) { std::lock_guard lock(kSecurityLock); const char* cpath = path.c_str(); @@ -434,6 +459,9 @@ status_t PrepareDir(const std::string& path, mode_t mode, uid_t uid, gid_t gid) freecon(secontext); } + if (res) return -errno; + if (attrs) res = SetAttrs(path, attrs); + if (res == 0) { return OK; } else { diff --git a/Utils.h b/Utils.h index 0def28b..769161a 100644 --- a/Utils.h +++ b/Utils.h @@ -66,7 +66,8 @@ int PrepareAppDirFromRoot(const std::string& path, const std::string& root, int bool fixupExisting); /* fs_prepare_dir wrapper that creates with SELinux context */ -status_t PrepareDir(const std::string& path, mode_t mode, uid_t uid, gid_t gid); +status_t PrepareDir(const std::string& path, mode_t mode, uid_t uid, gid_t gid, + unsigned int attrs = 0); /* Really unmounts the path, killing active processes along the way */ status_t ForceUnmount(const std::string& path); diff --git a/model/PrivateVolume.cpp b/model/PrivateVolume.cpp index 39a946c..1875b7b 100644 --- a/model/PrivateVolume.cpp +++ b/model/PrivateVolume.cpp @@ -166,11 +166,14 @@ status_t PrivateVolume::doMount() { RestoreconRecursive(mPath); + int attrs = 0; + if (!IsSdcardfsUsed()) attrs = FS_CASEFOLD_FL; + // Verify that common directories are ready to roll if (PrepareDir(mPath + "/app", 0771, AID_SYSTEM, AID_SYSTEM) || PrepareDir(mPath + "/user", 0711, AID_SYSTEM, AID_SYSTEM) || PrepareDir(mPath + "/user_de", 0711, AID_SYSTEM, AID_SYSTEM) || - PrepareDir(mPath + "/media", 0770, AID_MEDIA_RW, AID_MEDIA_RW) || + PrepareDir(mPath + "/media", 0770, AID_MEDIA_RW, AID_MEDIA_RW, attrs) || PrepareDir(mPath + "/media/0", 0770, AID_MEDIA_RW, AID_MEDIA_RW) || PrepareDir(mPath + "/local", 0751, AID_ROOT, AID_ROOT) || PrepareDir(mPath + "/local/tmp", 0771, AID_SHELL, AID_SHELL)) { From 763393644a0a6e361f26dfe26f6dc0021579f011 Mon Sep 17 00:00:00 2001 From: Alan Stokes Date: Thu, 19 Nov 2020 16:56:52 +0000 Subject: [PATCH 08/26] Enable improved user separation by default. This is already on for all Pixel devices with no problems observed. If this causes issues with a specific device (e.g. vendor apps being unable to access their data) it can be temporarily disabled by adding PRODUCT_PROPERTY_OVERRIDES += ro.vold.level_from_user=0 to the device.mk file. Please file a bug if that happens. Bug: 141677108 Test: presubmits Ignore-AOSP-First: Want to assess impact internally before CP to AOSP Change-Id: Ic9da534f1a5f4c9e3bd62ea5c09a3b11ebcb33e7 --- FsCrypt.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/FsCrypt.cpp b/FsCrypt.cpp index ff8c1f4..8f6ba9c 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -795,7 +795,7 @@ bool fscrypt_lock_user_key(userid_t user_id) { static bool prepare_subdirs(const std::string& action, const std::string& volume_uuid, userid_t user_id, int flags) { // TODO(b/141677108): Remove this & make it the default behavior - if (android::base::GetProperty("ro.vold.level_from_user", "0") == "1") { + if (android::base::GetProperty("ro.vold.level_from_user", "1") == "1") { flags |= android::os::IVold::STORAGE_FLAG_LEVEL_FROM_USER; } From 1bc5f5d4dd10cb5968620c02a50f49be921c6332 Mon Sep 17 00:00:00 2001 From: Nandana Dutt Date: Mon, 30 Nov 2020 11:07:25 +0000 Subject: [PATCH 09/26] Add CtsScopedStorageDeviceOnlyTest to vold presubmit BUG: 159593019 Test: presubmit Change-Id: Icd75637f4a168311fdaeca2b1f4f32b34ad20dbb --- TEST_MAPPING | 3 +++ 1 file changed, 3 insertions(+) diff --git a/TEST_MAPPING b/TEST_MAPPING index 15a4af4..49b2d60 100644 --- a/TEST_MAPPING +++ b/TEST_MAPPING @@ -6,6 +6,9 @@ { "name": "CtsScopedStorageHostTest" }, + { + "name": "CtsScopedStorageDeviceOnlyTest" + }, { "name": "AdoptableHostTest" } From f6642d6c3ef2b919283618bf4c5d8cb85927b2e7 Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Tue, 22 Dec 2020 11:25:21 +0000 Subject: [PATCH 10/26] Change mounting storage data and obb to on by default Bug: 148049767 Test: atest AdoptableHostTest Test: pass cts/cts_postsubmit_cf_stable-cloud-tf Ignore-AOSP-First: Merge conflicts Change-Id: I44b34308249712987ce65b00e6a78d0dc94e2e19 --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 4a77846..7c96ea2 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::~EmulatedVolume() {} From d40bf612deb56e7ec6ceb92539b39315786557fc Mon Sep 17 00:00:00 2001 From: Pulkit Sachdeva Date: Mon, 11 Jan 2021 21:12:05 +0000 Subject: [PATCH 11/26] Revert "Change mounting storage data and obb to on by default" Revert submission 13255890-enable_storage_data_iso Reason for revert: This change broke the CtsDownloadManagerInstaller test (https://screenshot.googleplex.com/ALNBECjxF38S76m.png) Reverted Changes: I2d6b690f5:Change mounting storage data and obb to on by defa... I44b343082:Change mounting storage data and obb to on by defa... Change-Id: I5ced80d42e84371c18b5d26ec551d6eed902a12b --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 7c96ea2..4a77846 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::~EmulatedVolume() {} From 0c0f83902d973d62e4dbb9cdf73440d5ef7b0b49 Mon Sep 17 00:00:00 2001 From: Yurii Zubrytskyi Date: Tue, 8 Dec 2020 07:33:37 -0800 Subject: [PATCH 12/26] IncFS: pass over the new .blocks_written IncFS control file Bug: 170231230 Test: incremental and PackageManager unit tests Ignore-AOSP-First: new IncFS API is an internal-first topic Change-Id: I7fccaf367d4b98294e2e6da4460792514147d954 --- VoldNativeService.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 03dee48..7a6ceff 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -920,6 +920,9 @@ binder::Status VoldNativeService::mountIncFs( _aidl_return->cmd.reset(unique_fd(fds[CMD].release())); _aidl_return->pendingReads.reset(unique_fd(fds[PENDING_READS].release())); _aidl_return->log.reset(unique_fd(fds[LOGS].release())); + if (fds[BLOCKS_WRITTEN].ok()) { + _aidl_return->blocksWritten.emplace(unique_fd(fds[BLOCKS_WRITTEN].release())); + } return Ok(); } @@ -936,7 +939,8 @@ binder::Status VoldNativeService::setIncFsMountOptions( ENFORCE_SYSTEM_OR_ROOT; auto incfsControl = - incfs::createControl(control.cmd.get(), control.pendingReads.get(), control.log.get()); + incfs::createControl(control.cmd.get(), control.pendingReads.get(), control.log.get(), + control.blocksWritten ? control.blocksWritten->get() : -1); auto cleanupFunc = [](auto incfsControl) { for (auto& fd : incfsControl->releaseFds()) { (void)fd.release(); From 61d1e01375a5dcb965eb6ffa66edd00e762f3265 Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Mon, 25 Jan 2021 12:19:32 +0000 Subject: [PATCH 13/26] Change mounting storage data and obb to on by default Bug: 148049767 Test: atest AdoptableHostTest Test: pass cts/cts_postsubmit_cf_stable-cloud-tf Ignore-AOSP-First: Another CL on same topic has merge conflict from aosp to internal master Change-Id: I46a0954489816df3651f2fc90d85b389fc38087f --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 4a77846..7c96ea2 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::~EmulatedVolume() {} From c6d94cf76e2c87f7fabd7eca9d638d100c118e62 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Mon, 15 Mar 2021 12:44:36 -0700 Subject: [PATCH 14/26] KeyStorage: improve logging for key generation The error messages that are printed when probing for rollback resistance support on a device that doesn't support rollback-resistant keys can make it sound like something is going wrong. Print a WARNING message afterwards to try to make it clear what is going on. Also adjust or add DEBUG messages when starting to generate each key so that it's easier to distinguish the log messages for different key generation operations. Bug: 182815123 Test: boot on device that doesn't support rollback-resistant keys and check log. Change-Id: I37a13eb5c1e839fb94581f3e7ec1cd8da0263d2b Merged-In: I37a13eb5c1e839fb94581f3e7ec1cd8da0263d2b --- KeyStorage.cpp | 40 +++++++++++++++++++++++----------------- KeyUtil.cpp | 2 ++ 2 files changed, 25 insertions(+), 17 deletions(-) diff --git a/KeyStorage.cpp b/KeyStorage.cpp index 457bb66..356d556 100644 --- a/KeyStorage.cpp +++ b/KeyStorage.cpp @@ -133,17 +133,33 @@ static void hashWithPrefix(char const* prefix, const std::string& tohash, std::s SHA512_Final(reinterpret_cast(&(*res)[0]), &c); } -static bool generateKeymasterKey(Keymaster& keymaster, const KeyAuthentication& auth, - const std::string& appId, std::string* key) { +// Generates a keymaster key, using rollback resistance if supported. +static bool generateKeymasterKey(Keymaster& keymaster, + const km::AuthorizationSetBuilder& paramBuilder, + std::string* key) { + auto paramsWithRollback = paramBuilder; + paramsWithRollback.Authorization(km::TAG_ROLLBACK_RESISTANCE); + + if (!keymaster.generateKey(paramsWithRollback, key)) { + LOG(WARNING) << "Failed to generate rollback-resistant key. This is expected if keymaster " + "doesn't support rollback resistance. Falling back to " + "non-rollback-resistant key."; + if (!keymaster.generateKey(paramBuilder, key)) return false; + } + return true; +} + +static bool generateKeyStorageKey(Keymaster& keymaster, const KeyAuthentication& auth, + const std::string& appId, std::string* key) { auto paramBuilder = km::AuthorizationSetBuilder() .AesEncryptionKey(AES_KEY_BYTES * 8) .GcmModeMinMacLen(GCM_MAC_BYTES * 8) .Authorization(km::TAG_APPLICATION_ID, km::support::blob2hidlVec(appId)); if (auth.token.empty()) { - LOG(DEBUG) << "Creating key that doesn't need auth token"; + LOG(DEBUG) << "Generating \"key storage\" key that doesn't need auth token"; paramBuilder.Authorization(km::TAG_NO_AUTH_REQUIRED); } else { - LOG(DEBUG) << "Auth token required for key"; + LOG(DEBUG) << "Generating \"key storage\" key that needs auth token"; if (auth.token.size() != sizeof(hw_auth_token_t)) { LOG(ERROR) << "Auth token should be " << sizeof(hw_auth_token_t) << " bytes, was " << auth.token.size() << " bytes"; @@ -155,13 +171,7 @@ static bool generateKeymasterKey(Keymaster& keymaster, const KeyAuthentication& paramBuilder.Authorization(km::TAG_USER_AUTH_TYPE, km::HardwareAuthenticatorType::PASSWORD); paramBuilder.Authorization(km::TAG_AUTH_TIMEOUT, AUTH_TIMEOUT); } - - auto paramsWithRollback = paramBuilder; - paramsWithRollback.Authorization(km::TAG_ROLLBACK_RESISTANCE); - - // Generate rollback-resistant key if possible. - return keymaster.generateKey(paramsWithRollback, key) || - keymaster.generateKey(paramBuilder, key); + return generateKeymasterKey(keymaster, paramBuilder, key); } bool generateWrappedStorageKey(KeyBuffer* key) { @@ -170,11 +180,7 @@ bool generateWrappedStorageKey(KeyBuffer* key) { std::string key_temp; auto paramBuilder = km::AuthorizationSetBuilder().AesEncryptionKey(AES_KEY_BYTES * 8); paramBuilder.Authorization(km::TAG_STORAGE_KEY); - auto paramsWithRollback = paramBuilder; - paramsWithRollback.Authorization(km::TAG_ROLLBACK_RESISTANCE); - if (!keymaster.generateKey(paramsWithRollback, &key_temp)) { - if (!keymaster.generateKey(paramBuilder, &key_temp)) return false; - } + if (!generateKeymasterKey(keymaster, paramBuilder, &key_temp)) return false; *key = KeyBuffer(key_temp.size()); memcpy(reinterpret_cast(key->data()), key_temp.c_str(), key->size()); return true; @@ -631,7 +637,7 @@ bool storeKey(const std::string& dir, const KeyAuthentication& auth, const KeyBu Keymaster keymaster; if (!keymaster) return false; std::string kmKey; - if (!generateKeymasterKey(keymaster, auth, appId, &kmKey)) return false; + if (!generateKeyStorageKey(keymaster, auth, appId, &kmKey)) return false; if (!writeStringToFile(kmKey, dir + "/" + kFn_keymaster_key_blob)) return false; km::AuthorizationSet keyParams; km::HardwareAuthToken authToken; diff --git a/KeyUtil.cpp b/KeyUtil.cpp index 9d01e1e..886054e 100644 --- a/KeyUtil.cpp +++ b/KeyUtil.cpp @@ -57,8 +57,10 @@ bool generateStorageKey(const KeyGeneration& gen, KeyBuffer* key) { LOG(ERROR) << "Cannot generate a wrapped key " << gen.keysize << " bytes long"; return false; } + LOG(DEBUG) << "Generating wrapped storage key"; return generateWrappedStorageKey(key); } else { + LOG(DEBUG) << "Generating standard storage key"; return randomKey(gen.keysize, key); } } From cc3b59f5aa60a49c82cb0285b26572a6178742ca Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Fri, 19 Mar 2021 14:21:46 +0000 Subject: [PATCH 15/26] Revert "Change mounting storage data and obb to on by default" Revert "Change mounting storage data and obb to on by default" Revert submission 13469849-turn_on_iso-sc-dev Reason for revert: Failing existing CTS b/182843583 Reverted Changes: If819ee161:Change mounting storage data and obb to on by defa... I46a095448:Change mounting storage data and obb to on by defa... Change-Id: Ic5156df1cac3a5ecd661b5f3bfa0095b2b767d5d --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 7c96ea2..4a77846 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::~EmulatedVolume() {} From dcfeaa1d7640f04bbf738f26b680895ca0b5ff38 Mon Sep 17 00:00:00 2001 From: Alex Buynytskyy Date: Thu, 1 Apr 2021 13:43:14 -0700 Subject: [PATCH 16/26] Adding an option to shorten the read timeout. E.g. during installation to protect the system. Ignore-AOSP-First: this depends on changes to framework and/or incfs and does not make sense without them. We'll merge it at a single large scale merge later. Bug: 160635296 Test: atest PackageManagerShellCommandTest PackageManagerShellCommandIncrementalTest IncrementalServiceTest PackageManagerServiceTest ChecksumsTest Change-Id: I5851e1e9dbc8e8c2b331c407002cf7133bf6e35a --- VoldNativeService.cpp | 7 +++++-- VoldNativeService.h | 2 +- binder/android/os/IVold.aidl | 2 +- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index a61d615..4cf1952 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -54,6 +54,7 @@ namespace vold { namespace { constexpr const char* kDump = "android.permission.DUMP"; +constexpr auto kIncFsReadNoTimeoutMs = 100; static binder::Status error(const std::string& msg) { PLOG(ERROR) << msg; @@ -955,6 +956,7 @@ binder::Status VoldNativeService::mountIncFs( auto control = incfs::mount(backingPath, targetDir, {.flags = IncFsMountFlags(flags), + // Mount with read timeouts. .defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS, // Mount with read logs disabled. .readLogBufferPages = 0}); @@ -981,7 +983,7 @@ binder::Status VoldNativeService::unmountIncFs(const std::string& dir) { binder::Status VoldNativeService::setIncFsMountOptions( const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs) { + bool enableReadLogs, bool enableReadTimeouts) { ENFORCE_SYSTEM_OR_ROOT; auto incfsControl = @@ -996,7 +998,8 @@ binder::Status VoldNativeService::setIncFsMountOptions( std::unique_ptr(&incfsControl, cleanupFunc); if (auto error = incfs::setOptions( incfsControl, - {.defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS, + {.defaultReadTimeoutMs = + enableReadTimeouts ? INCFS_DEFAULT_READ_TIMEOUT_MS : kIncFsReadNoTimeoutMs, .readLogBufferPages = enableReadLogs ? INCFS_DEFAULT_PAGE_READ_BUFFER_PAGES : 0}); error < 0) { return binder::Status::fromServiceSpecificError(error); diff --git a/VoldNativeService.h b/VoldNativeService.h index 123f127..8da91c0 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -165,7 +165,7 @@ class VoldNativeService : public BinderService, public os::Bn binder::Status unmountIncFs(const std::string& dir) override; binder::Status setIncFsMountOptions( const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs) override; + bool enableReadLogs, bool enableReadTimeouts) override; binder::Status bindMount(const std::string& sourceDir, const std::string& targetDir) override; binder::Status destroyDsuMetadataKey(const std::string& dsuSlot) override; diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index fd134c5..f09f1da 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -141,7 +141,7 @@ interface IVold { boolean incFsEnabled(); IncrementalFileSystemControlParcel mountIncFs(@utf8InCpp String backingPath, @utf8InCpp String targetDir, int flags); void unmountIncFs(@utf8InCpp String dir); - void setIncFsMountOptions(in IncrementalFileSystemControlParcel control, boolean enableReadLogs); + void setIncFsMountOptions(in IncrementalFileSystemControlParcel control, boolean enableReadLogs, boolean enableReadTimeouts); void bindMount(@utf8InCpp String sourceDir, @utf8InCpp String targetDir); void destroyDsuMetadataKey(@utf8InCpp String dsuSlot); From 1799debfd6561ca7348880bb59ad8c059f4891b0 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 6 Apr 2021 12:02:56 -0700 Subject: [PATCH 17/26] vold: add getUnlockedUsers() method to Binder interface This is needed so that system_server can remind itself about which users have their storage unlocked, if system_server is restarted due to a userspace reboot (soft restart). Bug: 146206679 Test: see I482ed8017f7bbc8f7d4fd5a2c0f58629317ce4ed Change-Id: I02f0494d827094bd41bcfe5f63c24e204b728595 --- FsCrypt.cpp | 8 ++++++++ FsCrypt.h | 2 ++ VoldNativeService.cpp | 8 ++++++++ VoldNativeService.h | 1 + binder/android/os/IVold.aidl | 1 + 5 files changed, 20 insertions(+) diff --git a/FsCrypt.cpp b/FsCrypt.cpp index cfa74e0..04def5c 100644 --- a/FsCrypt.cpp +++ b/FsCrypt.cpp @@ -730,6 +730,14 @@ bool fscrypt_fixate_newest_user_key_auth(userid_t user_id) { return true; } +std::vector fscrypt_get_unlocked_users() { + std::vector user_ids; + for (const auto& it : s_ce_policies) { + user_ids.push_back(it.first); + } + return user_ids; +} + // TODO: rename to 'install' for consistency, and take flags to know which keys to install bool fscrypt_unlock_user_key(userid_t user_id, int serial, const std::string& secret_hex) { LOG(DEBUG) << "fscrypt_unlock_user_key " << user_id << " serial=" << serial; diff --git a/FsCrypt.h b/FsCrypt.h index 96159d5..2946be5 100644 --- a/FsCrypt.h +++ b/FsCrypt.h @@ -15,6 +15,7 @@ */ #include +#include #include @@ -27,6 +28,7 @@ bool fscrypt_add_user_key_auth(userid_t user_id, int serial, const std::string& bool fscrypt_clear_user_key_auth(userid_t user_id, int serial, const std::string& secret); bool fscrypt_fixate_newest_user_key_auth(userid_t user_id); +std::vector fscrypt_get_unlocked_users(); bool fscrypt_unlock_user_key(userid_t user_id, int serial, const std::string& secret); bool fscrypt_lock_user_key(userid_t user_id); diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index a61d615..1429e54 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -764,6 +764,14 @@ binder::Status VoldNativeService::fixateNewestUserKeyAuth(int32_t userId) { return translateBool(fscrypt_fixate_newest_user_key_auth(userId)); } +binder::Status VoldNativeService::getUnlockedUsers(std::vector* _aidl_return) { + ENFORCE_SYSTEM_OR_ROOT; + ACQUIRE_CRYPT_LOCK; + + *_aidl_return = fscrypt_get_unlocked_users(); + return Ok(); +} + binder::Status VoldNativeService::unlockUserKey(int32_t userId, int32_t userSerial, const std::string& token, const std::string& secret) { diff --git a/VoldNativeService.h b/VoldNativeService.h index 123f127..33d0f3a 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -127,6 +127,7 @@ class VoldNativeService : public BinderService, public os::Bn const std::string& secret); binder::Status fixateNewestUserKeyAuth(int32_t userId); + binder::Status getUnlockedUsers(std::vector* _aidl_return); binder::Status unlockUserKey(int32_t userId, int32_t userSerial, const std::string& token, const std::string& secret); binder::Status lockUserKey(int32_t userId); diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index fd134c5..62685e5 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -102,6 +102,7 @@ interface IVold { @utf8InCpp String secret); void fixateNewestUserKeyAuth(int userId); + int[] getUnlockedUsers(); void unlockUserKey(int userId, int userSerial, @utf8InCpp String token, @utf8InCpp String secret); void lockUserKey(int userId); From 046e68abd6b4911d953091f39d0b3157b7041c02 Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Tue, 27 Apr 2021 12:46:02 -0700 Subject: [PATCH 18/26] [vold] pass sysfs_name to mount options Ignore-AOSP-First: Will cherry-pick to AOSP Test: manual BUG: 184844615 Change-Id: I216210132f49f55098c0f2d1b8d4e571b22cfcc4 --- VoldNativeService.cpp | 4 +++- VoldNativeService.h | 1 + binder/android/os/IVold.aidl | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 66c7d46..8d63a83 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -957,6 +957,7 @@ binder::Status VoldNativeService::incFsEnabled(bool* _aidl_return) { binder::Status VoldNativeService::mountIncFs( const std::string& backingPath, const std::string& targetDir, int32_t flags, + const std::string& sysfsName, ::android::os::incremental::IncrementalFileSystemControlParcel* _aidl_return) { ENFORCE_SYSTEM_OR_ROOT; CHECK_ARGUMENT_PATH(backingPath); @@ -967,7 +968,8 @@ binder::Status VoldNativeService::mountIncFs( // Mount with read timeouts. .defaultReadTimeoutMs = INCFS_DEFAULT_READ_TIMEOUT_MS, // Mount with read logs disabled. - .readLogBufferPages = 0}); + .readLogBufferPages = 0, + .sysfsName = sysfsName.c_str()}); if (!control) { return translate(-errno); } diff --git a/VoldNativeService.h b/VoldNativeService.h index c06e4b6..1414c38 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -162,6 +162,7 @@ class VoldNativeService : public BinderService, public os::Bn binder::Status incFsEnabled(bool* _aidl_return) override; binder::Status mountIncFs( const std::string& backingPath, const std::string& targetDir, int32_t flags, + const std::string& sysfsName, ::android::os::incremental::IncrementalFileSystemControlParcel* _aidl_return) override; binder::Status unmountIncFs(const std::string& dir) override; binder::Status setIncFsMountOptions( diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index 6a69804..f20faca 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -140,7 +140,7 @@ interface IVold { FileDescriptor openAppFuseFile(int uid, int mountId, int fileId, int flags); boolean incFsEnabled(); - IncrementalFileSystemControlParcel mountIncFs(@utf8InCpp String backingPath, @utf8InCpp String targetDir, int flags); + IncrementalFileSystemControlParcel mountIncFs(@utf8InCpp String backingPath, @utf8InCpp String targetDir, int flags, @utf8InCpp String sysfsName); void unmountIncFs(@utf8InCpp String dir); void setIncFsMountOptions(in IncrementalFileSystemControlParcel control, boolean enableReadLogs, boolean enableReadTimeouts); void bindMount(@utf8InCpp String sourceDir, @utf8InCpp String targetDir); From 4ae2c65f8da9df8e7f69a1a8950292d68d30d82a Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Tue, 23 Mar 2021 18:13:07 +0000 Subject: [PATCH 19/26] Change mounting storage data and obb flag to on by default Change mounting storage data and obb flag to on by default Test: unbundled/launcher/nexus_unit_test_multi_device_platform Test: atest android.appsecurity.cts.ExternalStorageHostTest Test: atest AdoptableHostTest Test: pass cts/cts_postsubmit_cf_stable-cloud-tf Bug: 148049767 Ignore-AOSP-First: Merge it along with other CLs, will cherry-pick to AOSP afterwards. Change-Id: I6391b7381699b4ffdbf715b67938bc3f79a5210c --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 9431f95..fe58555 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); } EmulatedVolume::~EmulatedVolume() {} From a58b535495f274f2a576150e4f574d90256d73dd Mon Sep 17 00:00:00 2001 From: Ricky Wai Date: Thu, 29 Apr 2021 17:47:28 +0100 Subject: [PATCH 20/26] Only kill apps with storage app data isolation enabled Originally it kills all the apps with obb and data mounted. Due to recent changes, all apps will have obb and data dirs mounted in default root namespace. Hence all apps will be killed by by KillProcessesWithMounts(). To fix this, we also check if the dir is mounted as tmpfs, as the default namespace one is bind mounted to lowerfs, which app data isolation is mounted as tmpfs, so we only kill the process that have obb dir mounted as tmpfs. Bug: 148049767 Test: Able to boot without warnings / errors Ignore-AOSP-First: Merge it along with other CLs, will cherry-pick to AOSP afterwards. Change-Id: I45d9a63ed47cbc27aebb63357a43f51ad62275db --- Process.cpp | 5 +++-- Process.h | 2 +- Utils.cpp | 10 +++++----- Utils.h | 4 ++-- model/EmulatedVolume.cpp | 4 +++- 5 files changed, 14 insertions(+), 11 deletions(-) diff --git a/Process.cpp b/Process.cpp index 62d51a2..79fe15d 100644 --- a/Process.cpp +++ b/Process.cpp @@ -84,7 +84,7 @@ static bool checkSymlink(const std::string& path, const std::string& prefix) { } // TODO: Refactor the code with KillProcessesWithOpenFiles(). -int KillProcessesWithMounts(const std::string& prefix, int signal) { +int KillProcessesWithTmpfsMounts(const std::string& prefix, int signal) { std::unordered_set pids; auto proc_d = std::unique_ptr(opendir("/proc"), closedir); @@ -112,7 +112,8 @@ int KillProcessesWithMounts(const std::string& prefix, int signal) { // Check if obb directory is mounted, and get all packages of mounted app data directory. mntent* mentry; while ((mentry = getmntent(fp.get())) != nullptr) { - if (android::base::StartsWith(mentry->mnt_dir, prefix)) { + if (mentry->mnt_fsname != nullptr && strncmp(mentry->mnt_fsname, "tmpfs", 5) == 0 + && android::base::StartsWith(mentry->mnt_dir, prefix)) { pids.insert(pid); break; } diff --git a/Process.h b/Process.h index a56b9ce..f3728b5 100644 --- a/Process.h +++ b/Process.h @@ -21,7 +21,7 @@ namespace android { namespace vold { int KillProcessesWithOpenFiles(const std::string& path, int signal, bool killFuseDaemon = true); -int KillProcessesWithMounts(const std::string& path, int signal); +int KillProcessesWithTmpfsMounts(const std::string& path, int signal); } // namespace vold } // namespace android diff --git a/Utils.cpp b/Utils.cpp index 9ff7920..b353197 100644 --- a/Utils.cpp +++ b/Utils.cpp @@ -499,25 +499,25 @@ status_t ForceUnmount(const std::string& path) { return -errno; } -status_t KillProcessesWithMountPrefix(const std::string& path) { - if (KillProcessesWithMounts(path, SIGINT) == 0) { +status_t KillProcessesWithTmpfsMountPrefix(const std::string& path) { + if (KillProcessesWithTmpfsMounts(path, SIGINT) == 0) { return OK; } if (sSleepOnUnmount) sleep(5); - if (KillProcessesWithMounts(path, SIGTERM) == 0) { + if (KillProcessesWithTmpfsMounts(path, SIGTERM) == 0) { return OK; } if (sSleepOnUnmount) sleep(5); - if (KillProcessesWithMounts(path, SIGKILL) == 0) { + if (KillProcessesWithTmpfsMounts(path, SIGKILL) == 0) { return OK; } if (sSleepOnUnmount) sleep(5); // Send SIGKILL a second time to determine if we've // actually killed everyone mount - if (KillProcessesWithMounts(path, SIGKILL) == 0) { + if (KillProcessesWithTmpfsMounts(path, SIGKILL) == 0) { return OK; } PLOG(ERROR) << "Failed to kill processes using " << path; diff --git a/Utils.h b/Utils.h index 4771593..a3316c3 100644 --- a/Utils.h +++ b/Utils.h @@ -78,8 +78,8 @@ status_t ForceUnmount(const std::string& path); /* Kills any processes using given path */ status_t KillProcessesUsingPath(const std::string& path); -/* Kills any processes using given mount prifix */ -status_t KillProcessesWithMountPrefix(const std::string& path); +/* Kills any processes using given tmpfs mount prifix */ +status_t KillProcessesWithTmpfsMountPrefix(const std::string& path); /* Creates bind mount from source to target */ status_t BindMount(const std::string& source, const std::string& target); diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index 9431f95..09a75b5 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -191,7 +191,9 @@ status_t EmulatedVolume::unmountFuseBindMounts() { // umount the whole Android/ dir. if (mAppDataIsolationEnabled) { std::string appObbDir(StringPrintf("%s/%d/Android/obb", getPath().c_str(), userId)); - KillProcessesWithMountPrefix(appObbDir); + // Here we assume obb/data dirs is mounted as tmpfs, then it must be caused by + // app data isolation. + KillProcessesWithTmpfsMountPrefix(appObbDir); } else { std::string androidDataTarget( StringPrintf("/mnt/user/%d/%s/%d/Android/data", userId, label.c_str(), userId)); From 0e53c1cdb0d0fa87e725ccde810c3efb90728afd Mon Sep 17 00:00:00 2001 From: Songchun Fan Date: Mon, 10 May 2021 16:19:38 -0700 Subject: [PATCH 21/26] [vold] pass along sysfs name in setOptions Ignore-AOSP-First: Will cherry-pick to AOSP BUG: 187308584 Test: atest CtsContentTestCases:android.content.pm.cts.PackageManagerShellCommandIncrementalTest#testInstallWithIdSigNoMissingPages Change-Id: Iacfe6b735458051f2848b1b766c2b00198b397d9 --- VoldNativeService.cpp | 5 +++-- VoldNativeService.h | 2 +- binder/android/os/IVold.aidl | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 8d63a83..d26758c 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -993,7 +993,7 @@ binder::Status VoldNativeService::unmountIncFs(const std::string& dir) { binder::Status VoldNativeService::setIncFsMountOptions( const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs, bool enableReadTimeouts) { + bool enableReadLogs, bool enableReadTimeouts, const std::string& sysfsName) { ENFORCE_SYSTEM_OR_ROOT; auto incfsControl = @@ -1010,7 +1010,8 @@ binder::Status VoldNativeService::setIncFsMountOptions( incfsControl, {.defaultReadTimeoutMs = enableReadTimeouts ? INCFS_DEFAULT_READ_TIMEOUT_MS : kIncFsReadNoTimeoutMs, - .readLogBufferPages = enableReadLogs ? INCFS_DEFAULT_PAGE_READ_BUFFER_PAGES : 0}); + .readLogBufferPages = enableReadLogs ? INCFS_DEFAULT_PAGE_READ_BUFFER_PAGES : 0, + .sysfsName = sysfsName.c_str()}); error < 0) { return binder::Status::fromServiceSpecificError(error); } diff --git a/VoldNativeService.h b/VoldNativeService.h index 1414c38..5fa04f5 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -167,7 +167,7 @@ class VoldNativeService : public BinderService, public os::Bn binder::Status unmountIncFs(const std::string& dir) override; binder::Status setIncFsMountOptions( const ::android::os::incremental::IncrementalFileSystemControlParcel& control, - bool enableReadLogs, bool enableReadTimeouts) override; + bool enableReadLogs, bool enableReadTimeouts, const std::string& sysfsName) override; binder::Status bindMount(const std::string& sourceDir, const std::string& targetDir) override; binder::Status destroyDsuMetadataKey(const std::string& dsuSlot) override; diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index f20faca..606f473 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -142,7 +142,7 @@ interface IVold { boolean incFsEnabled(); IncrementalFileSystemControlParcel mountIncFs(@utf8InCpp String backingPath, @utf8InCpp String targetDir, int flags, @utf8InCpp String sysfsName); void unmountIncFs(@utf8InCpp String dir); - void setIncFsMountOptions(in IncrementalFileSystemControlParcel control, boolean enableReadLogs, boolean enableReadTimeouts); + void setIncFsMountOptions(in IncrementalFileSystemControlParcel control, boolean enableReadLogs, boolean enableReadTimeouts, @utf8InCpp String sysfsName); void bindMount(@utf8InCpp String sourceDir, @utf8InCpp String targetDir); void destroyDsuMetadataKey(@utf8InCpp String dsuSlot); From 6cc9a1d3ddddd59c08ef2e17298f935e59200126 Mon Sep 17 00:00:00 2001 From: Wale Ogunwale Date: Thu, 13 May 2021 22:17:21 +0000 Subject: [PATCH 22/26] Revert "Change mounting storage data and obb flag to on by default" Revert "Change mounting storage data and obb flag to on by default" Revert "Remove storage app data isolation checking in CTS" Revert submission 14325408-enable_storage_iso_2 Reason for revert: b/187939590 Reverted Changes: I6391b7381:Change mounting storage data and obb flag to on by... Ic2f3d1be2:Remove storage app data isolation checking in CTS Iffa8339b1:Change mounting storage data and obb flag to on by... Bug: 187939590 Bug: 148049767 Change-Id: I8ef3e6fe0210bdf58e1292605ac1cc33a2eaafea --- model/EmulatedVolume.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/model/EmulatedVolume.cpp b/model/EmulatedVolume.cpp index ea8633b..6f21ff8 100644 --- a/model/EmulatedVolume.cpp +++ b/model/EmulatedVolume.cpp @@ -50,7 +50,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, int userId) mLabel = "emulated"; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const std::string& fsUuid, @@ -61,7 +61,7 @@ EmulatedVolume::EmulatedVolume(const std::string& rawPath, dev_t device, const s mLabel = fsUuid; mFuseMounted = false; mUseSdcardFs = IsSdcardfsUsed(); - mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, true); + mAppDataIsolationEnabled = base::GetBoolProperty(kVoldAppDataIsolationEnabled, false); } EmulatedVolume::~EmulatedVolume() {} From 2ddc1338d7a400c972cc2bc46b4aa9a377c22a1e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Thu, 24 Jun 2021 11:13:24 -0700 Subject: [PATCH 23/26] Ignore too-early earlyBootEnded on FDE devices Don't call IKeystoreMaintenance::earlyBootEnded() too early on FDE devices, so that keystore2 doesn't have to be restarted. Bug: 192090857 Test: Tested FDE on Cuttlefish, both first and non-first boots. Verified via log that earlyBootEnded is now called only when it should be, and that keystore2 no longer has to be restarted. Change-Id: I03f816db194a8276ad19ca99b3c8894e8a5fed23 (cherry picked from commit 4859e0ca0f7fc5da217e8b388da76ece41dd726e) Merged-In: I03f816db194a8276ad19ca99b3c8894e8a5fed23 --- VoldNativeService.cpp | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index d26758c..ef26fb6 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include @@ -939,10 +940,42 @@ static void initializeIncFs() { incfs::features(); } +// This is missing from the kernel UAPI headers. +#define ST_RDONLY 0x0001 + +// FDE devices run the post-fs-data trigger (and hence also earlyBootEnded) +// multiple times, sometimes prior to the real /data being mounted. That causes +// keystore2 to try to open a file in /data, causing it to panic or have to be +// killed by vold later, causing problems (vold failing to connect to keystore2, +// or keystore2 operations erroring out later). As a workaround to keep FDE +// working, ignore these too-early calls to earlyBootEnded. +// +// This can be removed when support for FDE is removed. +static bool IgnoreEarlyBootEnded() { + // The statfs("/data") below should be sufficient by itself, but to be safe + // we also explicitly return false on FBE devices. (This really should be + // ro.crypto.type != "block" for "non-FDE devices", but on FDE devices this + // is sometimes called before ro.crypto.type gets set.) + if (fscrypt_is_native()) return false; + + struct statfs buf; + if (statfs(DATA_MNT_POINT, &buf) != 0) { + PLOG(ERROR) << "statfs(\"/data\") failed"; + return false; + } + if (buf.f_type == TMPFS_MAGIC || (buf.f_flags & ST_RDONLY)) { + LOG(INFO) << "Ignoring earlyBootEnded since real /data isn't mounted yet"; + return true; + } + return false; +} + binder::Status VoldNativeService::earlyBootEnded() { ENFORCE_SYSTEM_OR_ROOT; ACQUIRE_LOCK; + if (IgnoreEarlyBootEnded()) return Ok(); + initializeIncFs(); Keymaster::earlyBootEnded(); return Ok(); From 0f74bd4811c506269c2139a40af1f66e75871cd8 Mon Sep 17 00:00:00 2001 From: Paul Crowley Date: Fri, 6 Aug 2021 15:16:10 -0700 Subject: [PATCH 24/26] Detect factory reset and deleteAllKeys Where metadata encryption is enabled, if there is no metadata encryption key present and we are generating one anew, then there has been a factory reset, and this is the first key to be generated. We then call deleteAllKeys to ensure data from before the factory reset is securely deleted. This shouldn't really be necessary; the factory reset call itself should be doing this. However there are currently three factory reset paths (settings, recovery, fastboot -w) and it is not clear that all three are doing this correctly on all devices. Obviously an attacker can prevent this code from being run by running a version of the OS that does not include this change; however, if the bootloader is locked, then keys will be version bound such that they will only work on locked devices with a sufficiently recent version of the OS. If every sufficiently recent signed version of the OS includes this change the attack is defeated. Bug: 187105270 Test: booted Cuttlefish twice, checked logs Ignore-AOSP-First: no merge path to this branch from AOSP. Merged-In: I9c5c547140e8b1bbffb9c1d215f75251f0f1354e Change-Id: I9c5c547140e8b1bbffb9c1d215f75251f0f1354e --- Keymaster.cpp | 13 +++++++++++++ Keymaster.h | 3 +++ MetadataCrypt.cpp | 11 +++++++++++ 3 files changed, 27 insertions(+) diff --git a/Keymaster.cpp b/Keymaster.cpp index 8038681..2314550 100644 --- a/Keymaster.cpp +++ b/Keymaster.cpp @@ -230,5 +230,18 @@ void Keymaster::earlyBootEnded() { logKeystore2ExceptionIfPresent(rc, "earlyBootEnded"); } +void Keymaster::deleteAllKeys() { + ::ndk::SpAIBinder binder(AServiceManager_getService(maintenance_service_name)); + auto maint_service = ks2_maint::IKeystoreMaintenance::fromBinder(binder); + + if (!maint_service) { + LOG(ERROR) << "Unable to connect to keystore2 maintenance service for deleteAllKeys"; + return; + } + + auto rc = maint_service->deleteAllKeys(); + logKeystore2ExceptionIfPresent(rc, "deleteAllKeys"); +} + } // namespace vold } // namespace android diff --git a/Keymaster.h b/Keymaster.h index 1100840..47bf4a2 100644 --- a/Keymaster.h +++ b/Keymaster.h @@ -127,6 +127,9 @@ class Keymaster { // be created or used. static void earlyBootEnded(); + // Tell all Keymint devices to delete all rollback-protected keys. + static void deleteAllKeys(); + private: std::shared_ptr securityLevel; DISALLOW_COPY_AND_ASSIGN(Keymaster); diff --git a/MetadataCrypt.cpp b/MetadataCrypt.cpp index dc50679..9038e8d 100644 --- a/MetadataCrypt.cpp +++ b/MetadataCrypt.cpp @@ -112,6 +112,17 @@ static bool read_key(const std::string& metadata_key_dir, const KeyGeneration& g auto dir = metadata_key_dir + "/key"; LOG(DEBUG) << "metadata_key_dir/key: " << dir; if (!MkdirsSync(dir, 0700)) return false; + if (!pathExists(dir)) { + auto delete_all = android::base::GetBoolProperty( + "ro.crypto.metadata_init_delete_all_keys.enabled", false); + if (delete_all) { + LOG(INFO) << "Metadata key does not exist, calling deleteAllKeys"; + Keymaster::deleteAllKeys(); + } else { + LOG(DEBUG) << "Metadata key does not exist but " + "ro.crypto.metadata_init_delete_all_keys.enabled is false"; + } + } auto temp = metadata_key_dir + "/tmp"; return retrieveOrGenerateKey(dir, temp, kEmptyAuthentication, gen, key); } From 2601eb7f8c241f9cf24510115e0a572819fd0514 Mon Sep 17 00:00:00 2001 From: "[6;7~" Date: Wed, 11 Aug 2021 12:46:49 -0700 Subject: [PATCH 25/26] Add ROLLBACK_RESISTANCE tag to key usage If KM is upgraded from a version that does not support rollback resistance to one that does, we really want our upgraded keys to include rollback resistance. By passing this tag in when we use the keys, we ensure that the tag is passed into the upgradeKey request whenever it is made, which some KM implementations can use to add rollback resistance to our keys. Bug: 187105270 Ignore-AOSP-First: no merge path to this branch from AOSP. Test: Manual Change-Id: I6154fe26a10b60cd686cc60dbc2e0a85c152f43b --- KeyStorage.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/KeyStorage.cpp b/KeyStorage.cpp index 472e6b1..93c5c29 100644 --- a/KeyStorage.cpp +++ b/KeyStorage.cpp @@ -379,7 +379,9 @@ static bool encryptWithKeymasterKey(Keymaster& keymaster, const std::string& dir const km::AuthorizationSet& keyParams, const KeyBuffer& message, std::string* ciphertext) { km::AuthorizationSet opParams = - km::AuthorizationSetBuilder().Authorization(km::TAG_PURPOSE, km::KeyPurpose::ENCRYPT); + km::AuthorizationSetBuilder() + .Authorization(km::TAG_ROLLBACK_RESISTANCE) + .Authorization(km::TAG_PURPOSE, km::KeyPurpose::ENCRYPT); km::AuthorizationSet outParams; auto opHandle = BeginKeymasterOp(keymaster, dir, keyParams, opParams, &outParams); if (!opHandle) return false; @@ -408,6 +410,7 @@ static bool decryptWithKeymasterKey(Keymaster& keymaster, const std::string& dir auto bodyAndMac = ciphertext.substr(GCM_NONCE_BYTES); auto opParams = km::AuthorizationSetBuilder() .Authorization(km::TAG_NONCE, nonce) + .Authorization(km::TAG_ROLLBACK_RESISTANCE) .Authorization(km::TAG_PURPOSE, km::KeyPurpose::DECRYPT); auto opHandle = BeginKeymasterOp(keymaster, dir, keyParams, opParams, nullptr); if (!opHandle) return false; From 2bab97c36830a568083ab60767817ef037c5367d Mon Sep 17 00:00:00 2001 From: Shawn Willden Date: Thu, 12 Aug 2021 01:03:12 +0000 Subject: [PATCH 26/26] Revert "Detect factory reset and deleteAllKeys" Revert "Add deleteAllKeys to IKeystoreMaintenance" Revert "Enable deleteAllKeys from vold" Revert "Allow vold to deleteAllKeys in Keystore" Revert submission 15521094-vold-deleteAllKeys Reason for revert: Causes infinite loop in Trusty KeyMint Reverted Changes: I9c5c54714:Detect factory reset and deleteAllKeys I2fb0e94db:Allow vold to deleteAllKeys in Keystore Id23f25c69:Add deleteAllKeys to IKeystoreMaintenance Ife779307d:Enable deleteAllKeys from vold I4312b9a11:Enable deleteAllKeys from vold Bug: 187105270 Change-Id: I8e2621bef234d0a59be422b8d1d8d52a91378a5e --- Keymaster.cpp | 13 ------------- Keymaster.h | 3 --- MetadataCrypt.cpp | 11 ----------- 3 files changed, 27 deletions(-) diff --git a/Keymaster.cpp b/Keymaster.cpp index 2314550..8038681 100644 --- a/Keymaster.cpp +++ b/Keymaster.cpp @@ -230,18 +230,5 @@ void Keymaster::earlyBootEnded() { logKeystore2ExceptionIfPresent(rc, "earlyBootEnded"); } -void Keymaster::deleteAllKeys() { - ::ndk::SpAIBinder binder(AServiceManager_getService(maintenance_service_name)); - auto maint_service = ks2_maint::IKeystoreMaintenance::fromBinder(binder); - - if (!maint_service) { - LOG(ERROR) << "Unable to connect to keystore2 maintenance service for deleteAllKeys"; - return; - } - - auto rc = maint_service->deleteAllKeys(); - logKeystore2ExceptionIfPresent(rc, "deleteAllKeys"); -} - } // namespace vold } // namespace android diff --git a/Keymaster.h b/Keymaster.h index 47bf4a2..1100840 100644 --- a/Keymaster.h +++ b/Keymaster.h @@ -127,9 +127,6 @@ class Keymaster { // be created or used. static void earlyBootEnded(); - // Tell all Keymint devices to delete all rollback-protected keys. - static void deleteAllKeys(); - private: std::shared_ptr securityLevel; DISALLOW_COPY_AND_ASSIGN(Keymaster); diff --git a/MetadataCrypt.cpp b/MetadataCrypt.cpp index 9038e8d..dc50679 100644 --- a/MetadataCrypt.cpp +++ b/MetadataCrypt.cpp @@ -112,17 +112,6 @@ static bool read_key(const std::string& metadata_key_dir, const KeyGeneration& g auto dir = metadata_key_dir + "/key"; LOG(DEBUG) << "metadata_key_dir/key: " << dir; if (!MkdirsSync(dir, 0700)) return false; - if (!pathExists(dir)) { - auto delete_all = android::base::GetBoolProperty( - "ro.crypto.metadata_init_delete_all_keys.enabled", false); - if (delete_all) { - LOG(INFO) << "Metadata key does not exist, calling deleteAllKeys"; - Keymaster::deleteAllKeys(); - } else { - LOG(DEBUG) << "Metadata key does not exist but " - "ro.crypto.metadata_init_delete_all_keys.enabled is false"; - } - } auto temp = metadata_key_dir + "/tmp"; return retrieveOrGenerateKey(dir, temp, kEmptyAuthentication, gen, key); }