From 3d9c19f2fd34cccac6fe09877ae9a6b4563a063e Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 29 Apr 2022 05:26:17 +0000 Subject: BrightnessTracker: don't create stuff directly in /data/system_de /data/system_de is only for per-user data; it *must* only contain per-user encrypted directories. Only vold should ever create anything directly in this directory. In preparation for removing system_server's write access to this directory (https://r.android.com/2078213), make BrightnessTracker store its brightness_events.xml file at /data/system/brightness_events.xml instead of /data/system_de/brightness_events.xml, and likewise for ambient_brightness_stats.xml. Migration happens lazily, except that the old files aren't ever deleted since the SELinux policy will no longer allow system_server to do that; the old files just stop being used. vold will need to handle the cleanup instead, or we could just leave the files around. Test: Flashed new build without wiping userdata. Log shows "Reading ambient_brightness_stats.xml from old location" as expected. Rebooted. Log message no longer appears, as expected. File /data/system/ambient_brightness_stats.xml exists, as expected. Bug: 156305599 Change-Id: I6e0948ac54ec9183348943234d505e7466042963 --- .../android/server/display/BrightnessTracker.java | 32 ++++++++++++++++++++-- .../server/display/BrightnessTrackerTest.java | 6 ++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/display/BrightnessTracker.java b/services/core/java/com/android/server/display/BrightnessTracker.java index 2c2a2bf24cfd..7d8a22a9b563 100644 --- a/services/core/java/com/android/server/display/BrightnessTracker.java +++ b/services/core/java/com/android/server/display/BrightnessTracker.java @@ -505,12 +505,36 @@ public class BrightnessTracker { } } + // Return the path to the given file, either the new path + // /data/system/$filename, or the old path /data/system_de/$filename if the + // file exists there but not at the new path. Only use this for EVENTS_FILE + // and AMBIENT_BRIGHTNESS_STATS_FILE. + // + // Explanation: this service previously incorrectly stored these two files + // directly in /data/system_de, instead of in /data/system where they should + // have been. As system_server no longer has write access to + // /data/system_de itself, these files were moved to /data/system. To + // lazily migrate the files, we simply read from the old path if it exists + // and the new one doesn't, and always write to the new path. Note that + // system_server doesn't have permission to delete the old files. + private AtomicFile getFileWithLegacyFallback(String filename) { + AtomicFile file = mInjector.getFile(filename); + if (file != null && !file.exists()) { + AtomicFile legacyFile = mInjector.getLegacyFile(filename); + if (legacyFile != null && legacyFile.exists()) { + Slog.i(TAG, "Reading " + filename + " from old location"); + return legacyFile; + } + } + return file; + } + private void readEvents() { synchronized (mEventsLock) { // Read might prune events so mark as dirty. mEventsDirty = true; mEvents.clear(); - final AtomicFile readFrom = mInjector.getFile(EVENTS_FILE); + final AtomicFile readFrom = getFileWithLegacyFallback(EVENTS_FILE); if (readFrom != null && readFrom.exists()) { FileInputStream input = null; try { @@ -528,7 +552,7 @@ public class BrightnessTracker { private void readAmbientBrightnessStats() { mAmbientBrightnessStatsTracker = new AmbientBrightnessStatsTracker(mUserManager, null); - final AtomicFile readFrom = mInjector.getFile(AMBIENT_BRIGHTNESS_STATS_FILE); + final AtomicFile readFrom = getFileWithLegacyFallback(AMBIENT_BRIGHTNESS_STATS_FILE); if (readFrom != null && readFrom.exists()) { FileInputStream input = null; try { @@ -1095,6 +1119,10 @@ public class BrightnessTracker { } public AtomicFile getFile(String filename) { + return new AtomicFile(new File(Environment.getDataSystemDirectory(), filename)); + } + + public AtomicFile getLegacyFile(String filename) { return new AtomicFile(new File(Environment.getDataSystemDeDirectory(), filename)); } diff --git a/services/tests/servicestests/src/com/android/server/display/BrightnessTrackerTest.java b/services/tests/servicestests/src/com/android/server/display/BrightnessTrackerTest.java index bdf94f3a2882..3151552fc4c9 100644 --- a/services/tests/servicestests/src/com/android/server/display/BrightnessTrackerTest.java +++ b/services/tests/servicestests/src/com/android/server/display/BrightnessTrackerTest.java @@ -1036,6 +1036,12 @@ public class BrightnessTrackerTest { return null; } + @Override + public AtomicFile getLegacyFile(String filename) { + // Don't have the test write / read from anywhere. + return null; + } + @Override public long currentTimeMillis() { return mCurrentTimeMillis; -- cgit v1.2.3-59-g8ed1b From 5454379320473a3a8005e4dc0a1e4c8a48350a57 Mon Sep 17 00:00:00 2001 From: Eric Biggers Date: Fri, 29 Apr 2022 05:26:18 +0000 Subject: Zygote: relabel to either system_userdir_file or system_data_file To hide other apps' data directories, zygote mounts tmpfs instances over various directories such as /data/user, then bind-mounts in the needed app directories. Files in tmpfs get the SELinux label "tmpfs" by default. After creating them, zygote relabels some of these "tmpfs" files to their normal labels; however, in every case the desired label was "u:object_r:system_data_file:s0", so zygote just copied the label of /data/user_de which happened to have this same label. With https://r.android.com/2078213, the /data/user_de directory, but not its subdirectories, will start being labeled as "u:object_r:system_userdir_file:s0". This would break the above-mentioned logic, as it would start assigning system_userdir_file to files that should be system_data_file. Therefore, update it to assign the appropriate type of [system_userdir_file, system_data_file]. Note that alternatively, it seems that we could just always use system_data_file, or just remove this relabeling code entirely since the sepolicy contains 'allow domain tmpfs:dir search' anyway. But as long as zygote is trying to set the normal labels, it should do it correctly. Test: Tested together with the sepolicy change https://r.android.com/2078213; see there for testing notes. Bug: 156305599 Change-Id: I325aa873cf216e33924d0508100fcc755f265cf2 --- core/jni/com_android_internal_os_Zygote.cpp | 41 ++++++++++++++++++----------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index f4f9f9437eb0..1122c20dda22 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -1153,8 +1153,8 @@ static void relabelDir(const char* path, const char* context, fail_fn_t fail_fn) } } -// Relabel all directories under a path non-recursively. -static void relabelAllDirs(const char* path, const char* context, fail_fn_t fail_fn) { +// Relabel the subdirectories and symlinks in the given directory, non-recursively. +static void relabelSubdirs(const char* path, const char* context, fail_fn_t fail_fn) { DIR* dir = opendir(path); if (dir == nullptr) { fail_fn(CREATE_ERROR("Failed to opendir %s", path)); @@ -1231,11 +1231,19 @@ static void isolateAppData(JNIEnv* env, const std::vector& merged_d snprintf(internalDePath, PATH_MAX, "/data/user_de"); snprintf(externalPrivateMountPath, PATH_MAX, "/mnt/expand"); - char* dataDataContext = nullptr; - if (getfilecon(internalDePath, &dataDataContext) < 0) { - fail_fn(CREATE_ERROR("Unable to getfilecon on %s %s", internalDePath, + // Get the "u:object_r:system_userdir_file:s0" security context. This can be + // gotten from several different places; we use /data/user. + char* dataUserdirContext = nullptr; + if (getfilecon(internalCePath, &dataUserdirContext) < 0) { + fail_fn(CREATE_ERROR("Unable to getfilecon on %s %s", internalCePath, strerror(errno))); } + // Get the "u:object_r:system_data_file:s0" security context. This can be + // gotten from several different places; we use /data/misc. + char* dataFileContext = nullptr; + if (getfilecon("/data/misc", &dataFileContext) < 0) { + fail_fn(CREATE_ERROR("Unable to getfilecon on /data/misc %s", strerror(errno))); + } MountAppDataTmpFs(internalLegacyCePath, fail_fn); MountAppDataTmpFs(internalCePath, fail_fn); @@ -1330,19 +1338,19 @@ static void isolateAppData(JNIEnv* env, const std::vector& merged_d // the file operations on tmpfs. If we set the label when we mount // tmpfs, SELinux will not happy as we are changing system_data_files. // Relabel dir under /data/user, including /data/user/0 - relabelAllDirs(internalCePath, dataDataContext, fail_fn); + relabelSubdirs(internalCePath, dataFileContext, fail_fn); // Relabel /data/user - relabelDir(internalCePath, dataDataContext, fail_fn); + relabelDir(internalCePath, dataUserdirContext, fail_fn); // Relabel /data/data - relabelDir(internalLegacyCePath, dataDataContext, fail_fn); + relabelDir(internalLegacyCePath, dataFileContext, fail_fn); - // Relabel dir under /data/user_de - relabelAllDirs(internalDePath, dataDataContext, fail_fn); + // Relabel subdirectories of /data/user_de + relabelSubdirs(internalDePath, dataFileContext, fail_fn); // Relabel /data/user_de - relabelDir(internalDePath, dataDataContext, fail_fn); + relabelDir(internalDePath, dataUserdirContext, fail_fn); // Relabel CE and DE dirs under /mnt/expand dir = opendir(externalPrivateMountPath); @@ -1355,14 +1363,15 @@ static void isolateAppData(JNIEnv* env, const std::vector& merged_d auto cePath = StringPrintf("%s/user", volPath.c_str()); auto dePath = StringPrintf("%s/user_de", volPath.c_str()); - relabelAllDirs(cePath.c_str(), dataDataContext, fail_fn); - relabelDir(cePath.c_str(), dataDataContext, fail_fn); - relabelAllDirs(dePath.c_str(), dataDataContext, fail_fn); - relabelDir(dePath.c_str(), dataDataContext, fail_fn); + relabelSubdirs(cePath.c_str(), dataFileContext, fail_fn); + relabelDir(cePath.c_str(), dataUserdirContext, fail_fn); + relabelSubdirs(dePath.c_str(), dataFileContext, fail_fn); + relabelDir(dePath.c_str(), dataUserdirContext, fail_fn); } closedir(dir); - freecon(dataDataContext); + freecon(dataUserdirContext); + freecon(dataFileContext); } static void insertPackagesToMergedList(JNIEnv* env, -- cgit v1.2.3-59-g8ed1b