Remove the overlapping uid range check

The overlapping uid range check will stop the uid update if the
existing range could cover the new updated rules. E.g. Update
range {0-10010, 10011-99999} to replace the existing range
{0-99999}.

The overlap uid range check was introduced in S as a safety
check, not for a bug fix. The uid range update relies on rules
updated from ConnectivityService. If unexpected duplicate uid
range rules are added to netd, they may mess up the ip rules.
Netd is nearly stateless that it does not know what is currently
set in IP rules but relies on the uid ranges stored in netd. So it
fundamentally requires the correctness of binder calls from the
upper layer. The uid ranges update is already correctly calculated
in ConnectivityService before updating to netd.

Alternative fix for the issue is trying to address the design
in ConnectivityService to remove the old uid range first before
adding new ranges. The problem here is that this may cause
packets leakage that packets are not subject to any uid rules
during the rule update. This would not be a proper fix for the
issue.

Also consider that this overlap check was started from S. This
may break the design in S devices if we only update design in
netd since netd is platform code unlike ConnectivityService that
is in a mainline module. This is OK because the possibility in
API surface to update uid range is started from T in VpnManager
that is also a platform code. The VPNs created from other
sources(legacy VPN or VpnService) would need to reconnect to
update the uid range. The new uid range update will result in
a new VPN network. The overlap check was not actually used in
S devices. That is, it's not really necessary in current design.
Remove this check does not actually introduce other legacy
issue, either.

Bug: 243900420
Test: cd system/netd ; atest
Test: manually test with VPN to exclude some uids and check the
      result
Change-Id: I4b26e4f5371cdce90b2595c82798ecc46963a92e
(cherry picked from commit 65bc4ea7f13dd8e870eaab2d5ad0e89e54a29c85)
Merged-In: I4b26e4f5371cdce90b2595c82798ecc46963a92e

8 files changed, 12 insertions, 49 deletions

diff --git a/server/Network.cpp b/server/Network.cpp
index 85f942f4..156cfb3e 100644
--- a/server/Network.cpp
+++ b/server/Network.cpp
@@ -117,18 +117,12 @@ void Network::removeFromUidRangeMap(const UidRanges& uidRanges, int32_t subPrior
     }
 }
 
-bool Network::canAddUidRanges(const UidRanges& uidRanges, int32_t subPriority) const {
+bool Network::canAddUidRanges(const UidRanges& uidRanges) const {
     if (uidRanges.overlapsSelf()) {
         ALOGE("uid range %s overlaps self", uidRanges.toString().c_str());
         return false;
     }
 
-    auto iter = mUidRangeMap.find(subPriority);
-    if (iter != mUidRangeMap.end() && uidRanges.overlaps(iter->second)) {
-        ALOGE("uid range %s overlaps priority %d %s", uidRanges.toString().c_str(), subPriority,
-              iter->second.toString().c_str());
-        return false;
-    }
     return true;
 }
 
diff --git a/server/Network.h b/server/Network.h
index e18e1cdb..dfead177 100644
--- a/server/Network.h
+++ b/server/Network.h
@@ -65,7 +65,7 @@ public:
 
 protected:
     explicit Network(unsigned netId, bool secure = false);
-    bool canAddUidRanges(const UidRanges& uidRanges, int32_t subPriority) const;
+    bool canAddUidRanges(const UidRanges& uidRanges) const;
 
     const unsigned mNetId;
     std::set<std::string> mInterfaces;
diff --git a/server/PhysicalNetwork.cpp b/server/PhysicalNetwork.cpp
index bb3f653d..68130641 100644
--- a/server/PhysicalNetwork.cpp
+++ b/server/PhysicalNetwork.cpp
@@ -165,7 +165,7 @@ int PhysicalNetwork::removeAsDefault() {
 }
 
 int PhysicalNetwork::addUsers(const UidRanges& uidRanges, int32_t subPriority) {
-    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges, subPriority)) {
+    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges)) {
         return -EINVAL;
     }
 
diff --git a/server/UidRanges.cpp b/server/UidRanges.cpp
index c90f30b9..765df322 100644
--- a/server/UidRanges.cpp
+++ b/server/UidRanges.cpp
@@ -145,25 +145,6 @@ bool UidRanges::overlapsSelf() const {
     return false;
 }
 
-// std::binary_search cannot do partial match. For example, an uid range x-y not only overlaps with
-// x-y, but also w-x, y-z, w-z, ...etc. Therefore, we need a specialized binary search.
-bool UidRanges::overlaps(const UidRanges& other) const {
-    for (const auto& target : other.getRanges()) {
-        int first = 0;
-        int end = mRanges.size() - 1;
-
-        while (first <= end) {
-            int middle = (first + end) / 2;
-            if (isOverlapped(mRanges[middle], target)) return true;
-            if (compUidRangeParcel(mRanges[middle], target))
-                first = middle + 1;
-            else
-                end = middle - 1;
-        }
-    }
-    return false;
-}
-
 std::string UidRanges::toString() const {
     std::string s("uids{ ");
     for (const auto &range : mRanges) {
diff --git a/server/UidRanges.h b/server/UidRanges.h
index 9123eb17..f20dc443 100644
--- a/server/UidRanges.h
+++ b/server/UidRanges.h
@@ -51,12 +51,10 @@ public:
 
     // check if 'mRanges' has uid overlap between elements.
     bool overlapsSelf() const;
-    // check if this object has uid overlap with the input object.
-    bool overlaps(const UidRanges& other) const;
+
     bool empty() const { return mRanges.empty(); }
 
   private:
-    // Keep it sorted. The overlaps() implements binary search, which requires a sorted data.
     std::vector<UidRangeParcel> mRanges;
 };
 
diff --git a/server/UnreachableNetwork.cpp b/server/UnreachableNetwork.cpp
index 68802251..dd6318c0 100644
--- a/server/UnreachableNetwork.cpp
+++ b/server/UnreachableNetwork.cpp
@@ -27,7 +27,7 @@ namespace net {
 UnreachableNetwork::UnreachableNetwork(unsigned netId) : Network(netId) {}
 
 int UnreachableNetwork::addUsers(const UidRanges& uidRanges, int32_t subPriority) {
-    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges, subPriority)) {
+    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges)) {
         return -EINVAL;
     }
 
diff --git a/server/VirtualNetwork.cpp b/server/VirtualNetwork.cpp
index 495fd161..e0f60407 100644
--- a/server/VirtualNetwork.cpp
+++ b/server/VirtualNetwork.cpp
@@ -33,7 +33,7 @@ VirtualNetwork::VirtualNetwork(unsigned netId, bool secure, bool excludeLocalRou
 VirtualNetwork::~VirtualNetwork() {}
 
 int VirtualNetwork::addUsers(const UidRanges& uidRanges, int32_t subPriority) {
-    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges, subPriority)) {
+    if (!isValidSubPriority(subPriority) || !canAddUidRanges(uidRanges)) {
         return -EINVAL;
     }
 
diff --git a/tests/binder_test.cpp b/tests/binder_test.cpp
index dae1af3e..d9d7cecc 100644
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -3999,7 +3999,7 @@ void expectUnreachableError(uid_t uid, unsigned netId, int selectionMode) {
 
 }  // namespace
 
-// Verify whether API reject overlapped UID ranges
+// Verify how the API handle overlapped UID ranges
 TEST_F(NetdBinderTest, PerAppDefaultNetwork_OverlappedUidRanges) {
     const auto& config = makeNativeNetworkConfig(APP_DEFAULT_NETID, NativeNetworkType::PHYSICAL,
                                                  INetd::PERMISSION_NONE, false, false);
@@ -4013,28 +4013,23 @@ TEST_F(NetdBinderTest, PerAppDefaultNetwork_OverlappedUidRanges) {
     binder::Status status;
     status = mNetd->networkAddUidRanges(APP_DEFAULT_NETID,
                                         {makeUidRangeParcel(BASE_UID + 1, BASE_UID + 1)});
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
+    EXPECT_TRUE(status.isOk());
 
     status = mNetd->networkAddUidRanges(APP_DEFAULT_NETID,
                                         {makeUidRangeParcel(BASE_UID + 9, BASE_UID + 10)});
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
+    EXPECT_TRUE(status.isOk());
 
     status = mNetd->networkAddUidRanges(APP_DEFAULT_NETID,
                                         {makeUidRangeParcel(BASE_UID + 11, BASE_UID + 11)});
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
+    EXPECT_TRUE(status.isOk());
 
     status = mNetd->networkAddUidRanges(APP_DEFAULT_NETID,
                                         {makeUidRangeParcel(BASE_UID + 12, BASE_UID + 13)});
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
+    EXPECT_TRUE(status.isOk());
 
     status = mNetd->networkAddUidRanges(APP_DEFAULT_NETID,
                                         {makeUidRangeParcel(BASE_UID + 9, BASE_UID + 13)});
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
+    EXPECT_TRUE(status.isOk());
 
     std::vector<UidRangeParcel> selfOverlappedUidRanges = {
             makeUidRangeParcel(BASE_UID + 20, BASE_UID + 20),
@@ -4730,11 +4725,6 @@ TEST_F(NetdBinderTest, UidRangeSubPriority_ValidateInputs) {
     uidRangeConfig.subPriority = SUB_PRIORITY_2;
     EXPECT_TRUE(mNetd->networkAddUidRangesParcel(uidRangeConfig).isOk());
 
-    // For a single network, identical UID ranges with the same priority is invalid.
-    status = mNetd->networkAddUidRangesParcel(uidRangeConfig);
-    EXPECT_FALSE(status.isOk());
-    EXPECT_EQ(EINVAL, status.serviceSpecificErrorCode());
-
     // Overlapping ranges is invalid.
     uidRangeConfig.uidRanges = {makeUidRangeParcel(BASE_UID + 1, BASE_UID + 1),
                                 makeUidRangeParcel(BASE_UID + 1, BASE_UID + 1)};