[security] Make frro iteration thread-safe
Add a lock over all frro iterator methods to make sure the
iterator isn't changed or removed from under the iteration
call while it's running.
+ Also introduce an ID ensuring the caller is the same for
all the acquire/next/release methods
+ a bit of moves where they were supposed to be from the start
Bug: 245727875
Test: manual + presubmits
Change-Id: Ie585b6d113ebddee491c9ee433f070bed71a57cc
diff --git a/cmds/idmap2/idmap2d/Idmap2Service.cpp b/cmds/idmap2/idmap2d/Idmap2Service.cpp
index 320e147..4431164 100644
--- a/cmds/idmap2/idmap2d/Idmap2Service.cpp
+++ b/cmds/idmap2/idmap2d/Idmap2Service.cpp
@@ -23,6 +23,7 @@
#include <cstring>
#include <filesystem>
#include <fstream>
+#include <limits>
#include <memory>
#include <ostream>
#include <string>
@@ -301,28 +302,42 @@
return ok();
}
-Status Idmap2Service::acquireFabricatedOverlayIterator() {
+Status Idmap2Service::acquireFabricatedOverlayIterator(int32_t* _aidl_return) {
+ std::lock_guard l(frro_iter_mutex_);
if (frro_iter_.has_value()) {
LOG(WARNING) << "active ffro iterator was not previously released";
}
frro_iter_ = std::filesystem::directory_iterator(kIdmapCacheDir);
+ if (frro_iter_id_ == std::numeric_limits<int32_t>::max()) {
+ frro_iter_id_ = 0;
+ } else {
+ ++frro_iter_id_;
+ }
+ *_aidl_return = frro_iter_id_;
return ok();
}
-Status Idmap2Service::releaseFabricatedOverlayIterator() {
+Status Idmap2Service::releaseFabricatedOverlayIterator(int32_t iteratorId) {
+ std::lock_guard l(frro_iter_mutex_);
if (!frro_iter_.has_value()) {
LOG(WARNING) << "no active ffro iterator to release";
+ } else if (frro_iter_id_ != iteratorId) {
+ LOG(WARNING) << "incorrect iterator id in a call to release";
} else {
- frro_iter_ = std::nullopt;
+ frro_iter_.reset();
}
return ok();
}
-Status Idmap2Service::nextFabricatedOverlayInfos(
+Status Idmap2Service::nextFabricatedOverlayInfos(int32_t iteratorId,
std::vector<os::FabricatedOverlayInfo>* _aidl_return) {
+ std::lock_guard l(frro_iter_mutex_);
+
constexpr size_t kMaxEntryCount = 100;
if (!frro_iter_.has_value()) {
return error("no active frro iterator");
+ } else if (frro_iter_id_ != iteratorId) {
+ return error("incorrect iterator id in a call to next");
}
size_t count = 0;
@@ -330,22 +345,22 @@
auto entry_iter_end = end(*frro_iter_);
for (; entry_iter != entry_iter_end && count < kMaxEntryCount; ++entry_iter) {
auto& entry = *entry_iter;
- if (!entry.is_regular_file() || !android::IsFabricatedOverlay(entry.path())) {
+ if (!entry.is_regular_file() || !android::IsFabricatedOverlay(entry.path().native())) {
continue;
}
- const auto overlay = FabricatedOverlayContainer::FromPath(entry.path());
+ const auto overlay = FabricatedOverlayContainer::FromPath(entry.path().native());
if (!overlay) {
LOG(WARNING) << "Failed to open '" << entry.path() << "': " << overlay.GetErrorMessage();
continue;
}
- const auto info = (*overlay)->GetManifestInfo();
+ auto info = (*overlay)->GetManifestInfo();
os::FabricatedOverlayInfo out_info;
- out_info.packageName = info.package_name;
- out_info.overlayName = info.name;
- out_info.targetPackageName = info.target_package;
- out_info.targetOverlayable = info.target_name;
+ out_info.packageName = std::move(info.package_name);
+ out_info.overlayName = std::move(info.name);
+ out_info.targetPackageName = std::move(info.target_package);
+ out_info.targetOverlayable = std::move(info.target_name);
out_info.path = entry.path();
_aidl_return->emplace_back(std::move(out_info));
count++;
diff --git a/cmds/idmap2/idmap2d/Idmap2Service.h b/cmds/idmap2/idmap2d/Idmap2Service.h
index c61e4bc..cc8cc5f 100644
--- a/cmds/idmap2/idmap2d/Idmap2Service.h
+++ b/cmds/idmap2/idmap2d/Idmap2Service.h
@@ -26,7 +26,10 @@
#include <filesystem>
#include <memory>
+#include <mutex>
+#include <optional>
#include <string>
+#include <variant>
#include <vector>
namespace android::os {
@@ -60,11 +63,11 @@
binder::Status deleteFabricatedOverlay(const std::string& overlay_path,
bool* _aidl_return) override;
- binder::Status acquireFabricatedOverlayIterator() override;
+ binder::Status acquireFabricatedOverlayIterator(int32_t* _aidl_return) override;
- binder::Status releaseFabricatedOverlayIterator() override;
+ binder::Status releaseFabricatedOverlayIterator(int32_t iteratorId) override;
- binder::Status nextFabricatedOverlayInfos(
+ binder::Status nextFabricatedOverlayInfos(int32_t iteratorId,
std::vector<os::FabricatedOverlayInfo>* _aidl_return) override;
binder::Status dumpIdmap(const std::string& overlay_path, std::string* _aidl_return) override;
@@ -74,7 +77,9 @@
// be able to be recalculated if idmap2 dies and restarts.
std::unique_ptr<idmap2::TargetResourceContainer> framework_apk_cache_;
+ int32_t frro_iter_id_ = 0;
std::optional<std::filesystem::directory_iterator> frro_iter_;
+ std::mutex frro_iter_mutex_;
template <typename T>
using MaybeUniquePtr = std::variant<std::unique_ptr<T>, T*>;
diff --git a/cmds/idmap2/idmap2d/aidl/services/android/os/IIdmap2.aidl b/cmds/idmap2/idmap2d/aidl/services/android/os/IIdmap2.aidl
index 0059cf2..2bbfba9 100644
--- a/cmds/idmap2/idmap2d/aidl/services/android/os/IIdmap2.aidl
+++ b/cmds/idmap2/idmap2d/aidl/services/android/os/IIdmap2.aidl
@@ -41,9 +41,9 @@
@nullable FabricatedOverlayInfo createFabricatedOverlay(in FabricatedOverlayInternal overlay);
boolean deleteFabricatedOverlay(@utf8InCpp String path);
- void acquireFabricatedOverlayIterator();
- void releaseFabricatedOverlayIterator();
- List<FabricatedOverlayInfo> nextFabricatedOverlayInfos();
+ int acquireFabricatedOverlayIterator();
+ void releaseFabricatedOverlayIterator(int iteratorId);
+ List<FabricatedOverlayInfo> nextFabricatedOverlayInfos(int iteratorId);
@utf8InCpp String dumpIdmap(@utf8InCpp String overlayApkPath);
}
diff --git a/services/core/java/com/android/server/om/IdmapDaemon.java b/services/core/java/com/android/server/om/IdmapDaemon.java
index bea6168..56390a9 100644
--- a/services/core/java/com/android/server/om/IdmapDaemon.java
+++ b/services/core/java/com/android/server/om/IdmapDaemon.java
@@ -217,6 +217,7 @@
synchronized List<FabricatedOverlayInfo> getFabricatedOverlayInfos() {
final ArrayList<FabricatedOverlayInfo> allInfos = new ArrayList<>();
Connection c = null;
+ int iteratorId = -1;
try {
c = connect();
final IIdmap2 service = c.getIdmap2();
@@ -225,9 +226,9 @@
return Collections.emptyList();
}
- service.acquireFabricatedOverlayIterator();
+ iteratorId = service.acquireFabricatedOverlayIterator();
List<FabricatedOverlayInfo> infos;
- while (!(infos = service.nextFabricatedOverlayInfos()).isEmpty()) {
+ while (!(infos = service.nextFabricatedOverlayInfos(iteratorId)).isEmpty()) {
allInfos.addAll(infos);
}
return allInfos;
@@ -235,8 +236,8 @@
Slog.wtf(TAG, "failed to get all fabricated overlays", e);
} finally {
try {
- if (c.getIdmap2() != null) {
- c.getIdmap2().releaseFabricatedOverlayIterator();
+ if (c.getIdmap2() != null && iteratorId != -1) {
+ c.getIdmap2().releaseFabricatedOverlayIterator(iteratorId);
}
} catch (RemoteException e) {
// ignore