From 7e79a43a72fd06e491063f7291358f8c6e86cda9 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Tue, 1 Mar 2022 21:19:18 +0000 Subject: [PATCH] Remove broken code for mounting encrypted OBB files Mounting encrypted OBB files has never worked reliably across devices, partly due to its reliance on Twofish encryption support in the kernel. This is because Twofish support (CONFIG_CRYPTO_TWOFISH) has never been required or even recommended for Android. It has never been enabled in GKI, but even before GKI it wasn't required or recommended. Moreover, this is now the only Android feature that still uses dm-crypt (CONFIG_DM_CRYPT), and some devices don't have that enabled either. Therefore, it appears that this feature is unused. That's perhaps not surprising, considering that the documentation for OBBs (https://developer.android.com/google/play/expansion-files) says that they are deprecated, and also it explains OBBs as being app files that are opaque to the platform; the ability of the platform to mount OBBs that happen to be in a particular format is never mentioned. That means that OBB mounting is probably rarely used even with unencrypted OBBs. Finally, the usefulness of OBBs having their own encryption layer (in addition to what the platform already provides via FBE) is not clear either, especially with such an unusual choice of cipher. To avoid the confusion that is being caused by having the broken code for mounting encrypted OBBs still sitting around, let's remove it. Test: atest StorageManagerTest # on Cuttlefish Test: atest StorageManagerIntegrationTest # on Cuttlefish Bug: 216475849 Change-Id: Iaef32cce90f95ea745ba2b143f89e66f533f3479 --- Android.bp | 1 - Devmapper.cpp | 100 ----------------------------------- Devmapper.h | 31 ----------- VoldNativeService.cpp | 7 +-- VoldNativeService.h | 4 +- VolumeManager.cpp | 8 ++- VolumeManager.h | 3 +- binder/android/os/IVold.aidl | 3 +- model/ObbVolume.cpp | 30 +---------- model/ObbVolume.h | 5 +- 10 files changed, 12 insertions(+), 180 deletions(-) delete mode 100644 Devmapper.cpp delete mode 100644 Devmapper.h diff --git a/Android.bp b/Android.bp index 1550264..38abdb9 100644 --- a/Android.bp +++ b/Android.bp @@ -115,7 +115,6 @@ cc_library_static { "Benchmark.cpp", "Checkpoint.cpp", "CryptoType.cpp", - "Devmapper.cpp", "EncryptInplace.cpp", "FileDeviceUtils.cpp", "FsCrypt.cpp", diff --git a/Devmapper.cpp b/Devmapper.cpp deleted file mode 100644 index 00fb4b3..0000000 --- a/Devmapper.cpp +++ /dev/null @@ -1,100 +0,0 @@ -/* - * Copyright (C) 2008 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#define ATRACE_TAG ATRACE_TAG_PACKAGE_MANAGER - -#include -#include -#include -#include -#include -#include - -#include -#include - -#include - -#include -#include -#include -#include -#include - -#include "Devmapper.h" - -using android::base::StringPrintf; -using namespace android::dm; - -static const char* kVoldPrefix = "vold:"; - -int Devmapper::create(const char* name_raw, const char* loopFile, const char* key, - unsigned long numSectors, char* ubuffer, size_t len) { - auto& dm = DeviceMapper::Instance(); - auto name_string = StringPrintf("%s%s", kVoldPrefix, name_raw); - - DmTable table; - table.Emplace(0, numSectors, "twofish", key, 0, loopFile, 0); - - if (!dm.CreateDevice(name_string, table)) { - LOG(ERROR) << "Failed to create device-mapper device " << name_string; - return -1; - } - - std::string path; - if (!dm.GetDmDevicePathByName(name_string, &path)) { - LOG(ERROR) << "Failed to get device-mapper device path for " << name_string; - return -1; - } - snprintf(ubuffer, len, "%s", path.c_str()); - return 0; -} - -int Devmapper::destroy(const char* name_raw) { - auto& dm = DeviceMapper::Instance(); - - auto name_string = StringPrintf("%s%s", kVoldPrefix, name_raw); - if (!dm.DeleteDevice(name_string)) { - if (errno != ENXIO) { - PLOG(ERROR) << "Failed DM_DEV_REMOVE"; - } - return -1; - } - return 0; -} - -int Devmapper::destroyAll() { - ATRACE_NAME("Devmapper::destroyAll"); - - auto& dm = DeviceMapper::Instance(); - std::vector devices; - if (!dm.GetAvailableDevices(&devices)) { - LOG(ERROR) << "Failed to get dm devices"; - return -1; - } - - for (const auto& device : devices) { - if (android::base::StartsWith(device.name(), kVoldPrefix)) { - LOG(DEBUG) << "Tearing down stale dm device named " << device.name(); - if (!dm.DeleteDevice(device.name())) { - if (errno != ENXIO) { - PLOG(WARNING) << "Failed to destroy dm device named " << device.name(); - } - } - } - } - return 0; -} diff --git a/Devmapper.h b/Devmapper.h deleted file mode 100644 index 9d4896e..0000000 --- a/Devmapper.h +++ /dev/null @@ -1,31 +0,0 @@ -/* - * Copyright (C) 2008 The Android Open Source Project - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -#ifndef _DEVMAPPER_H -#define _DEVMAPPER_H - -#include -#include - -class Devmapper { - public: - static int create(const char* name, const char* loopFile, const char* key, - unsigned long numSectors, char* buffer, size_t len); - static int destroy(const char* name); - static int destroyAll(); -}; - -#endif diff --git a/VoldNativeService.cpp b/VoldNativeService.cpp index 1033af9..576717c 100644 --- a/VoldNativeService.cpp +++ b/VoldNativeService.cpp @@ -414,16 +414,13 @@ binder::Status VoldNativeService::fixupAppDir(const std::string& path, int32_t a return translate(VolumeManager::Instance()->fixupAppDir(path, appUid)); } -binder::Status VoldNativeService::createObb(const std::string& sourcePath, - const std::string& sourceKey, int32_t ownerGid, +binder::Status VoldNativeService::createObb(const std::string& sourcePath, int32_t ownerGid, std::string* _aidl_return) { ENFORCE_SYSTEM_OR_ROOT; CHECK_ARGUMENT_PATH(sourcePath); - CHECK_ARGUMENT_HEX(sourceKey); ACQUIRE_LOCK; - return translate( - VolumeManager::Instance()->createObb(sourcePath, sourceKey, ownerGid, _aidl_return)); + return translate(VolumeManager::Instance()->createObb(sourcePath, ownerGid, _aidl_return)); } binder::Status VoldNativeService::destroyObb(const std::string& volId) { diff --git a/VoldNativeService.h b/VoldNativeService.h index 49bcbaa..58301f5 100644 --- a/VoldNativeService.h +++ b/VoldNativeService.h @@ -73,8 +73,8 @@ class VoldNativeService : public BinderService, public os::Bn binder::Status setupAppDir(const std::string& path, int32_t appUid); binder::Status fixupAppDir(const std::string& path, int32_t appUid); - binder::Status createObb(const std::string& sourcePath, const std::string& sourceKey, - int32_t ownerGid, std::string* _aidl_return); + binder::Status createObb(const std::string& sourcePath, int32_t ownerGid, + std::string* _aidl_return); binder::Status destroyObb(const std::string& volId); binder::Status createStubVolume(const std::string& sourcePath, const std::string& mountPath, diff --git a/VolumeManager.cpp b/VolumeManager.cpp index 9311321..bc51042 100644 --- a/VolumeManager.cpp +++ b/VolumeManager.cpp @@ -55,7 +55,6 @@ #include #include "AppFuseUtil.h" -#include "Devmapper.h" #include "FsCrypt.h" #include "Loop.h" #include "NetlinkManager.h" @@ -179,7 +178,6 @@ int VolumeManager::start() { // directories that we own, in case we crashed. unmountAll(); - Devmapper::destroyAll(); Loop::destroyAll(); // Assume that we always have an emulated volume on internal @@ -1075,8 +1073,8 @@ int VolumeManager::fixupAppDir(const std::string& path, int32_t appUid) { return setupAppDir(path, appUid, true /* fixupExistingOnly */); } -int VolumeManager::createObb(const std::string& sourcePath, const std::string& sourceKey, - int32_t ownerGid, std::string* outVolId) { +int VolumeManager::createObb(const std::string& sourcePath, int32_t ownerGid, + std::string* outVolId) { int id = mNextObbId++; std::string lowerSourcePath; @@ -1114,7 +1112,7 @@ int VolumeManager::createObb(const std::string& sourcePath, const std::string& s } auto vol = std::shared_ptr( - new android::vold::ObbVolume(id, lowerSourcePath, sourceKey, ownerGid)); + new android::vold::ObbVolume(id, lowerSourcePath, ownerGid)); vol->create(); mObbVolumes.push_back(vol); diff --git a/VolumeManager.h b/VolumeManager.h index 3573b1a..a8117c9 100644 --- a/VolumeManager.h +++ b/VolumeManager.h @@ -186,8 +186,7 @@ class VolumeManager { // Called before zygote starts to ensure dir exists so zygote can bind mount them. int ensureAppDirsCreated(const std::vector& paths, int32_t appUid); - int createObb(const std::string& path, const std::string& key, int32_t ownerGid, - std::string* outVolId); + int createObb(const std::string& path, int32_t ownerGid, std::string* outVolId); int destroyObb(const std::string& volId); int createStubVolume(const std::string& sourcePath, const std::string& mountPath, diff --git a/binder/android/os/IVold.aidl b/binder/android/os/IVold.aidl index c72ceea..cec0593 100644 --- a/binder/android/os/IVold.aidl +++ b/binder/android/os/IVold.aidl @@ -60,8 +60,7 @@ interface IVold { void fixupAppDir(@utf8InCpp String path, int appUid); void ensureAppDirsCreated(in @utf8InCpp String[] paths, int appUid); - @utf8InCpp String createObb(@utf8InCpp String sourcePath, @utf8InCpp String sourceKey, - int ownerGid); + @utf8InCpp String createObb(@utf8InCpp String sourcePath, int ownerGid); void destroyObb(@utf8InCpp String volId); void fstrim(int fstrimFlags, IVoldTaskListener listener); diff --git a/model/ObbVolume.cpp b/model/ObbVolume.cpp index 21479c4..b64c1ba 100644 --- a/model/ObbVolume.cpp +++ b/model/ObbVolume.cpp @@ -15,7 +15,6 @@ */ #include "ObbVolume.h" -#include "Devmapper.h" #include "Loop.h" #include "Utils.h" #include "VoldUtil.h" @@ -39,12 +38,10 @@ using android::base::StringPrintf; namespace android { namespace vold { -ObbVolume::ObbVolume(int id, const std::string& sourcePath, const std::string& sourceKey, - gid_t ownerGid) +ObbVolume::ObbVolume(int id, const std::string& sourcePath, gid_t ownerGid) : VolumeBase(Type::kObb) { setId(StringPrintf("obb:%d", id)); mSourcePath = sourcePath; - mSourceKey = sourceKey; mOwnerGid = ownerGid; } @@ -55,36 +52,13 @@ status_t ObbVolume::doCreate() { PLOG(ERROR) << getId() << " failed to create loop"; return -1; } - - if (!mSourceKey.empty()) { - uint64_t nr_sec = 0; - if (GetBlockDev512Sectors(mLoopPath, &nr_sec) != OK) { - PLOG(ERROR) << getId() << " failed to get loop size"; - return -1; - } - - char tmp[PATH_MAX]; - if (Devmapper::create(getId().c_str(), mLoopPath.c_str(), mSourceKey.c_str(), nr_sec, tmp, - PATH_MAX)) { - PLOG(ERROR) << getId() << " failed to create dm"; - return -1; - } - mDmPath = tmp; - mMountPath = mDmPath; - } else { - mMountPath = mLoopPath; - } return OK; } status_t ObbVolume::doDestroy() { - if (!mDmPath.empty() && Devmapper::destroy(getId().c_str())) { - PLOG(WARNING) << getId() << " failed to destroy dm"; - } if (!mLoopPath.empty() && Loop::destroyByDevice(mLoopPath.c_str())) { PLOG(WARNING) << getId() << " failed to destroy loop"; } - mDmPath.clear(); mLoopPath.clear(); return OK; } @@ -98,7 +72,7 @@ status_t ObbVolume::doMount() { return -1; } // clang-format off - if (android::vold::vfat::Mount(mMountPath, path, true, false, true, + if (android::vold::vfat::Mount(mLoopPath, path, true, false, true, 0, mOwnerGid, 0227, false)) { // clang-format on PLOG(ERROR) << getId() << " failed to mount"; diff --git a/model/ObbVolume.h b/model/ObbVolume.h index 8f7ee94..bfcd3d2 100644 --- a/model/ObbVolume.h +++ b/model/ObbVolume.h @@ -29,7 +29,7 @@ namespace vold { */ class ObbVolume : public VolumeBase { public: - ObbVolume(int id, const std::string& sourcePath, const std::string& sourceKey, gid_t ownerGid); + ObbVolume(int id, const std::string& sourcePath, gid_t ownerGid); virtual ~ObbVolume(); protected: @@ -40,12 +40,9 @@ class ObbVolume : public VolumeBase { private: std::string mSourcePath; - std::string mSourceKey; gid_t mOwnerGid; std::string mLoopPath; - std::string mDmPath; - std::string mMountPath; DISALLOW_COPY_AND_ASSIGN(ObbVolume); };