commit 09175beff3b521c7cd4de01b71098fdd564630aa
author Lorenzo Colitti <lorenzo@google.com> Fri Nov 17 14:01:21 2017 +0900
committer Lorenzo Colitti <lorenzo@google.com> Wed Nov 22 01:21:20 2017 +0900
tree 2c2a396b0c3ec25c87c814466c1313a89471d126
parent 0a786e10a760572e6c94ccba3ce42ad88dd532ae

Don't check for UPDATE_DEVICE_STATS if the caller is the system server.

Checking permissions requires a binder IPC to the system server.
If the system server's thread pool is full, and all the threads
in the pool blocked waiting for us to complete, we will deadlock.

Fix this by not doing the permission check if the caller is
the system server. From a security perspective there is no
difference because the system server always has all permissions.

Bug: 69389492
Test: walleye builds, boots
Test: netd_{unit,integration}_test pass
Change-Id: Ica8c389d943562b3a7c5b5c2e985b2fa052bdb1e

server/Android.mk

diff --git a/server/Android.mk b/server/Android.mk
index 51d2918..d91d063 100644
--- a/server/Android.mk
+++ b/server/Android.mk
@@ -87,6 +87,7 @@
         libnetdaidl \
         libnetutils \
         libnetdutils \
+        libselinux \
         libssl \
         libsysutils \
         libbase \

server/FwmarkServer.cpp

diff --git a/server/FwmarkServer.cpp b/server/FwmarkServer.cpp
index 5fe4cbe..32b856a 100644
--- a/server/FwmarkServer.cpp
+++ b/server/FwmarkServer.cpp
@@ -24,6 +24,7 @@
 #include "resolv_netid.h"
 
 #include <netinet/in.h>
+#include <selinux/selinux.h>
 #include <sys/socket.h>
 #include <unistd.h>
 #include <utils/String16.h>
@@ -36,10 +37,33 @@
 namespace android {
 namespace net {
 
-const char UPDATE_DEVICE_STATS[] = "android.permission.UPDATE_DEVICE_STATS";
+constexpr const char *UPDATE_DEVICE_STATS = "android.permission.UPDATE_DEVICE_STATS";
+constexpr const char *SYSTEM_SERVER_CONTEXT = "u:r:system_server:s0";
+
+bool isSystemServer(SocketClient* client) {
+    if (client->getUid() != AID_SYSTEM) {
+        return false;
+    }
+
+    char *context;
+    if (getpeercon(client->getSocket(), &context)) {
+        return false;
+    }
+
+    // We can't use context_new and context_type_get as they're private to libselinux. So just do
+    // a string match instead.
+    bool ret = !strcmp(context, SYSTEM_SERVER_CONTEXT);
+    freecon(context);
+
+    return ret;
+}
 
 bool hasUpdateDeviceStatsPermission(SocketClient* client) {
-    return checkPermission(String16(UPDATE_DEVICE_STATS), client->getPid(), client->getUid());
+    // If the caller is the system server, allow without any further checks.
+    // Otherwise, if the system server's binder thread pool is full, and all the threads are
+    // blocked on a thread that's waiting for us to complete, we deadlock. http://b/69389492
+    return isSystemServer(client) ||
+           checkPermission(String16(UPDATE_DEVICE_STATS), client->getPid(), client->getUid());
 }
 
 FwmarkServer::FwmarkServer(NetworkController* networkController, EventReporter* eventReporter,