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
diff --git a/server/Network.cpp b/server/Network.cpp
index 85f942f..156cfb3 100644
--- a/server/Network.cpp
+++ b/server/Network.cpp
@@ -117,18 +117,12 @@
}
}
-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 e18e1cd..dfead17 100644
--- a/server/Network.h
+++ b/server/Network.h
@@ -65,7 +65,7 @@
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 bb3f653..6813064 100644
--- a/server/PhysicalNetwork.cpp
+++ b/server/PhysicalNetwork.cpp
@@ -165,7 +165,7 @@
}
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 c90f30b..765df32 100644
--- a/server/UidRanges.cpp
+++ b/server/UidRanges.cpp
@@ -145,25 +145,6 @@
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 9123eb1..f20dc44 100644
--- a/server/UidRanges.h
+++ b/server/UidRanges.h
@@ -51,12 +51,10 @@
// 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 6880225..dd6318c 100644
--- a/server/UnreachableNetwork.cpp
+++ b/server/UnreachableNetwork.cpp
@@ -27,7 +27,7 @@
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 495fd16..e0f6040 100644
--- a/server/VirtualNetwork.cpp
+++ b/server/VirtualNetwork.cpp
@@ -33,7 +33,7 @@
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 dae1af3..d9d7cec 100644
--- a/tests/binder_test.cpp
+++ b/tests/binder_test.cpp
@@ -3999,7 +3999,7 @@
} // 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 @@
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 @@
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)};
