From fd5562b0a50d170fef4346dff6852c36719848eb Mon Sep 17 00:00:00 2001 From: Akilesh Kailash Date: Tue, 25 Jan 2022 07:05:31 +0000 Subject: [PATCH] init: Wait for snapuserd before starting second stage This is a race between init process and bionic libc initialization of snapuserd. init->fork() ----------------> SecondStageMain() -> PropertyInit() | | v execveat ---> __libc_init_common() -> __system_properties_init() (snapuserd) When init process calls PropertyInit(), /dev/__properties__ directory is created. When bionic libc of snapuserd daemon invokes __system_properties_init _after_ init process PropertyInit() function is invoked, libc will try to initialize the property by reading /system/etc/selinux/plat_property_contexts. Since any reads on /system has to be served by snapuserd, this specific read from libc cannot be serviced leading to deadlock. Reproduce the race by inducing a sleep of 1500ms just before execveat() so that init process calls PropertyInit() before bionic libc initialization. This leads to deadlock immediately and with additional kernel instrumentation with debug logs confirms the failure: ====================================================== init: Relaunched snapuserd with pid: 428 ext4_file_open: SNAPUSERD: path /system/etc/selinux/plat_property_contexts - Pid: 428 comm 8 ext4_file_read_iter: SNAPUSERD for path: /system/etc/selinux/plat_property_contexts pid: 428 comm 8 [ 25.418043][ T428] ext4_file_read_iter+0x3dc/0x3e0 [ 25.423000][ T428] vfs_read+0x2e0/0x354 [ 25.426986][ T428] ksys_read+0x7c/0xec [ 25.430894][ T428] __arm64_sys_read+0x20/0x30 [ 25.435419][ T428] el0_svc_common.llvm.17612735770287389485+0xd0/0x1e0 [ 25.442095][ T428] do_el0_svc+0x28/0xa0 [ 25.446100][ T428] el0_svc+0x14/0x24 [ 25.449825][ T428] el0_sync_handler+0x88/0xec [ 25.454343][ T428] el0_sync+0x1c0/0x200 ===================================================== Fix: Before starting init second stage, we will wait for snapuserd daemon to be up and running. We do a simple probe by reading system partition. This read will eventually be serviced by daemon confirming that daemon is up and running. Furthermore, we are still in the kernel domain and sepolicy has not been enforced yet. Thus, access to these device mapper block devices are ok even though we may see audit logs. Note that daemon will re-initialize the __system_property_init() as part of WaitForSocket() call. This is subtle but important; since bionic libc initialized had failed silently, it is important that this re-initialization is done. Bug: 207298357 Test: Induce the failure by explicitly delaying the call of execveat(). With fix, no issues observed. Tested incremental OTA on pixel ~15 times. Signed-off-by: Akilesh Kailash Change-Id: I86c2de977de052bfe9dcdc002dcbd9026601d0f3 --- .../user-space-merge/snapuserd_server.cpp | 9 ++- init/snapuserd_transition.cpp | 58 +++++++++++++++++++ init/snapuserd_transition.h | 1 + 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp index a79e3e13a..eb647047c 100644 --- a/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp +++ b/fs_mgr/libsnapshot/snapuserd/user-space-merge/snapuserd_server.cpp @@ -599,8 +599,13 @@ bool UserSnapshotServer::WaitForSocket() { return false; } - // We must re-initialize property service access, since we launched before - // second-stage init. + // This initialization of system property is important. When daemon is + // launched post selinux transition (before init second stage), + // bionic libc initializes system property as part of __libc_init_common(); + // however that initialization fails silently given that fact that we don't + // have /dev/__properties__ setup which is created at init second stage. + // + // At this point, we have the handlers setup and is safe to setup property. __system_properties_init(); if (!android::base::WaitForProperty("snapuserd.proxy_ready", "true")) { diff --git a/init/snapuserd_transition.cpp b/init/snapuserd_transition.cpp index e11510ee8..5deaf3156 100644 --- a/init/snapuserd_transition.cpp +++ b/init/snapuserd_transition.cpp @@ -32,6 +32,7 @@ #include #include #include +#include #include #include #include @@ -247,6 +248,56 @@ void SnapuserdSelinuxHelper::FinishTransition() { } } +/* + * Before starting init second stage, we will wait + * for snapuserd daemon to be up and running; bionic libc + * may read /system/etc/selinux/plat_property_contexts file + * before invoking main() function. This will happen if + * init initializes property during second stage. Any access + * to /system without snapuserd daemon will lead to a deadlock. + * + * Thus, we do a simple probe by reading system partition. This + * read will eventually be serviced by daemon confirming that + * daemon is up and running. Furthermore, we are still in the kernel + * domain and sepolicy has not been enforced yet. Thus, access + * to these device mapper block devices are ok even though + * we may see audit logs. + */ +bool SnapuserdSelinuxHelper::TestSnapuserdIsReady() { + std::string dev = "/dev/block/mapper/system"s + fs_mgr_get_slot_suffix(); + android::base::unique_fd fd(open(dev.c_str(), O_RDONLY | O_DIRECT)); + if (fd < 0) { + PLOG(ERROR) << "open " << dev << " failed"; + return false; + } + + void* addr; + ssize_t page_size = getpagesize(); + if (posix_memalign(&addr, page_size, page_size) < 0) { + PLOG(ERROR) << "posix_memalign with page size " << page_size; + return false; + } + + std::unique_ptr buffer(addr, ::free); + + int iter = 0; + while (iter < 10) { + ssize_t n = TEMP_FAILURE_RETRY(pread(fd.get(), buffer.get(), page_size, 0)); + if (n < 0) { + // Wait for sometime before retry + std::this_thread::sleep_for(100ms); + } else if (n == page_size) { + return true; + } else { + LOG(ERROR) << "pread returned: " << n << " from: " << dev << " expected: " << page_size; + } + + iter += 1; + } + + return false; +} + void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { auto fd = GetRamdiskSnapuserdFd(); if (!fd) { @@ -268,6 +319,13 @@ void SnapuserdSelinuxHelper::RelaunchFirstStageSnapuserd() { setenv(kSnapuserdFirstStagePidVar, std::to_string(pid).c_str(), 1); LOG(INFO) << "Relaunched snapuserd with pid: " << pid; + + if (!TestSnapuserdIsReady()) { + PLOG(FATAL) << "snapuserd daemon failed to launch"; + } else { + LOG(INFO) << "snapuserd daemon is up and running"; + } + return; } diff --git a/init/snapuserd_transition.h b/init/snapuserd_transition.h index be22afd20..557d10587 100644 --- a/init/snapuserd_transition.h +++ b/init/snapuserd_transition.h @@ -56,6 +56,7 @@ class SnapuserdSelinuxHelper final { private: void RelaunchFirstStageSnapuserd(); void ExecSnapuserd(); + bool TestSnapuserdIsReady(); std::unique_ptr sm_; BlockDevInitializer block_dev_init_;