From de2906af9c7b48cc27d94824342c7a96048d3e1c Mon Sep 17 00:00:00 2001 From: Peter Collingbourne Date: Thu, 25 Mar 2021 11:46:44 -0700 Subject: [PATCH] Reset PAC keys on thread creation instead of on zygote fork. Resetting PAC keys on fork appears to lead to a number of problems. One problem is that we are constrained in where we can run C++ code after forking, and with ART those places are implementation-defined. For example, in app zygotes, ART turns out to insert "interpreter frames" in the stack trace. Returning into these interpreter frames may lead to crashes due to failing the ROP protection check on return. It seems better to reset keys on thread creation instead. We only need to reset IA because only this key needs to be reset for reverse-edge PAC, and resetting the other keys may be incompatible with future ABIs. Chrome (and potentially other applications) has a sandbox that prevents the use of the prctl, so we restrict its use to applications targeting S and above. Bug: 183024045 Change-Id: I1e6502a7d7df319d424e2b0f653aad9a343ae71b --- core/jni/com_android_internal_os_Zygote.cpp | 21 ------------------- core/jni/com_android_internal_os_Zygote.h | 14 ------------- ...ndroid_internal_os_ZygoteCommandBuffer.cpp | 1 - 3 files changed, 36 deletions(-) diff --git a/core/jni/com_android_internal_os_Zygote.cpp b/core/jni/com_android_internal_os_Zygote.cpp index 836074f1d5f74..6ac43bd30973b 100644 --- a/core/jni/com_android_internal_os_Zygote.cpp +++ b/core/jni/com_android_internal_os_Zygote.cpp @@ -1015,21 +1015,6 @@ static void ClearUsapTable() { gUsapPoolCount = 0; } -NO_PAC_FUNC -static void PAuthKeyChange(JNIEnv* env) { -#ifdef __aarch64__ - unsigned long int hwcaps = getauxval(AT_HWCAP); - if (hwcaps & HWCAP_PACA) { - const unsigned long key_mask = PR_PAC_APIAKEY | PR_PAC_APIBKEY | - PR_PAC_APDAKEY | PR_PAC_APDBKEY | PR_PAC_APGAKEY; - if (prctl(PR_PAC_RESET_KEYS, key_mask, 0, 0, 0) != 0) { - ALOGE("Failed to change the PAC keys: %s", strerror(errno)); - RuntimeAbort(env, __LINE__, "PAC key change failed."); - } - } -#endif -} - // Create an app data directory over tmpfs overlayed CE / DE storage, and bind mount it // from the actual app data directory in data mirror. static bool createAndMountAppData(std::string_view package_name, @@ -1980,7 +1965,6 @@ void zygote::ZygoteFailure(JNIEnv* env, } // Utility routine to fork a process from the zygote. -NO_PAC_FUNC pid_t zygote::ForkCommon(JNIEnv* env, bool is_system_server, const std::vector& fds_to_close, const std::vector& fds_to_ignore, @@ -2035,7 +2019,6 @@ pid_t zygote::ForkCommon(JNIEnv* env, bool is_system_server, } // The child process. - PAuthKeyChange(env); PreApplicationInit(); // Clean up any descriptors which must be closed immediately @@ -2067,7 +2050,6 @@ static void com_android_internal_os_Zygote_nativePreApplicationInit(JNIEnv*, jcl PreApplicationInit(); } -NO_PAC_FUNC static jint com_android_internal_os_Zygote_nativeForkAndSpecialize( JNIEnv* env, jclass, jint uid, jint gid, jintArray gids, jint runtime_flags, jobjectArray rlimits, jint mount_external, jstring se_info, jstring nice_name, @@ -2117,7 +2099,6 @@ static jint com_android_internal_os_Zygote_nativeForkAndSpecialize( return pid; } -NO_PAC_FUNC static jint com_android_internal_os_Zygote_nativeForkSystemServer( JNIEnv* env, jclass, uid_t uid, gid_t gid, jintArray gids, jint runtime_flags, jobjectArray rlimits, jlong permitted_capabilities, @@ -2189,7 +2170,6 @@ static jint com_android_internal_os_Zygote_nativeForkSystemServer( * @param is_priority_fork Controls the nice level assigned to the newly created process * @return child pid in the parent, 0 in the child */ -NO_PAC_FUNC static jint com_android_internal_os_Zygote_nativeForkApp(JNIEnv* env, jclass, jint read_pipe_fd, @@ -2204,7 +2184,6 @@ static jint com_android_internal_os_Zygote_nativeForkApp(JNIEnv* env, args_known == JNI_TRUE, is_priority_fork == JNI_TRUE, true); } -NO_PAC_FUNC int zygote::forkApp(JNIEnv* env, int read_pipe_fd, int write_pipe_fd, diff --git a/core/jni/com_android_internal_os_Zygote.h b/core/jni/com_android_internal_os_Zygote.h index d2da91476bc7f..b87396cbd5f5c 100644 --- a/core/jni/com_android_internal_os_Zygote.h +++ b/core/jni/com_android_internal_os_Zygote.h @@ -20,18 +20,6 @@ #define LOG_TAG "Zygote" #define ATRACE_TAG ATRACE_TAG_DALVIK -/* Functions in the callchain during the fork shall not be protected with - Armv8.3-A Pointer Authentication, otherwise child will not be able to return. */ -#ifdef __ARM_FEATURE_PAC_DEFAULT -#ifdef __ARM_FEATURE_BTI_DEFAULT -#define NO_PAC_FUNC __attribute__((target("branch-protection=bti"))) -#else -#define NO_PAC_FUNC __attribute__((target("branch-protection=none"))) -#endif /* __ARM_FEATURE_BTI_DEFAULT */ -#else /* !__ARM_FEATURE_PAC_DEFAULT */ -#define NO_PAC_FUNC -#endif /* __ARM_FEATURE_PAC_DEFAULT */ - #include #include #include @@ -42,7 +30,6 @@ namespace android { namespace zygote { -NO_PAC_FUNC pid_t ForkCommon(JNIEnv* env,bool is_system_server, const std::vector& fds_to_close, const std::vector& fds_to_ignore, @@ -57,7 +44,6 @@ pid_t ForkCommon(JNIEnv* env,bool is_system_server, * communication is required. Is_priority_fork should be true if this is on the app startup * critical path. Purge specifies that unused pages should be purged before the fork. */ -NO_PAC_FUNC int forkApp(JNIEnv* env, int read_pipe_fd, int write_pipe_fd, diff --git a/core/jni/com_android_internal_os_ZygoteCommandBuffer.cpp b/core/jni/com_android_internal_os_ZygoteCommandBuffer.cpp index 011e8f8f1b8cd..24fef4881e4c3 100644 --- a/core/jni/com_android_internal_os_ZygoteCommandBuffer.cpp +++ b/core/jni/com_android_internal_os_ZygoteCommandBuffer.cpp @@ -365,7 +365,6 @@ void com_android_internal_os_ZygoteCommandBuffer_nativeReadFullyAndReset(JNIEnv* // We only process fork commands if the peer uid matches expected_uid. // For every fork command after the first, we check that the requested uid is at // least minUid. -NO_PAC_FUNC jboolean com_android_internal_os_ZygoteCommandBuffer_nativeForkRepeatedly( JNIEnv* env, jclass,