[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

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 @@ Status Idmap2Service::createFabricatedOverlay(
   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 @@ Status Idmap2Service::nextFabricatedOverlayInfos(
   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++;

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 @@ class Idmap2Service : public BinderService<Idmap2Service>, public BnIdmap2 {
   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 @@ class Idmap2Service : public BinderService<Idmap2Service>, public BnIdmap2 {
   // 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*>;

cmds/idmap2/idmap2d/aidl/services/android/os/IIdmap2.aidl

@@ -41,9 +41,9 @@ interface IIdmap2 {
   @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);
 }

services/core/java/com/android/server/om/IdmapDaemon.java

@@ -217,6 +217,7 @@ class IdmapDaemon {
     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 @@ class IdmapDaemon {
                 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 @@ class IdmapDaemon {
             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