diff --git a/zygiskd/src/utils.c b/zygiskd/src/utils.c index d4ba865..ceae36c 100644 --- a/zygiskd/src/utils.c +++ b/zygiskd/src/utils.c @@ -592,13 +592,7 @@ bool parse_mountinfo(const char *restrict pid, struct mountinfos *restrict mount return true; } -enum mns_umount_state { - Complete, - NotComplete, - Error -}; - -enum mns_umount_state unmount_root(struct root_impl impl) { +bool umount_root(struct root_impl impl) { /* INFO: We are already in the target pid mount namespace, so actually, when we use self here, we meant its pid. */ @@ -606,16 +600,9 @@ enum mns_umount_state unmount_root(struct root_impl impl) { if (!parse_mountinfo("self", &mounts)) { LOGE("Failed to parse mountinfo\n"); - return Error; + return false; } - /* INFO: Implementations like Magisk Kitsune will mount MagiskSU when boot is completed, - so if we cache the clean mount done before the boot is completed, it will get - it mounted later and hence it will leak mounts. To avoid that we will detect - if implementation is Kitsune, and if so, see if /system/bin... is mounted, - if not, it won't cache this namespace. */ - bool magiskSU_umounted = false; - switch (impl.impl) { case None: { break; } case Multiple: { break; } @@ -656,7 +643,7 @@ enum mns_umount_state unmount_root(struct root_impl impl) { free(targets_to_unmount); free_mounts(&mounts); - return Error; + return false; } targets_to_unmount[num_targets - 1] = mount.target; @@ -695,7 +682,6 @@ enum mns_umount_state unmount_root(struct root_impl impl) { if (strncmp(mount.target, "/debug_ramdisk", strlen("/debug_ramdisk")) == 0) should_unmount = true; if (strncmp(mount.target, "/data/adb/modules", strlen("/data/adb/modules")) == 0) should_unmount = true; if (strncmp(mount.root, "/adb/modules", strlen("/adb/modules")) == 0) should_unmount = true; - if (strncmp(mount.target, "/system/bin", strlen("/system/bin")) == 0) should_unmount = true; if (!should_unmount) continue; @@ -707,13 +693,10 @@ enum mns_umount_state unmount_root(struct root_impl impl) { free(targets_to_unmount); free_mounts(&mounts); - return Error; + return false; } targets_to_unmount[num_targets - 1] = mount.target; - - if (impl.impl == Magisk && strncmp(mount.target, "/system/bin", strlen("/system/bin")) == 0) - magiskSU_umounted = true; } for (size_t i = num_targets; i > 0; i--) { @@ -732,7 +715,7 @@ enum mns_umount_state unmount_root(struct root_impl impl) { free_mounts(&mounts); - return (impl.impl == Magisk && !magiskSU_umounted) ? NotComplete : Complete; + return true; } int save_mns_fd(int pid, enum MountNamespaceState mns_state, struct root_impl impl) { @@ -770,22 +753,20 @@ int save_mns_fd(int pid, enum MountNamespaceState mns_state, struct root_impl im if (switch_mount_namespace(pid) == false) { LOGE("Failed to switch mount namespace\n"); - write_uint8_t(writer, (uint8_t)Error); + write_uint8_t(writer, (uint8_t)false); close(writer); _exit(1); } - enum mns_umount_state umount_state = Complete; - if (mns_state == Clean) { unshare(CLONE_NEWNS); - umount_state = unmount_root(impl); - if (umount_state == Error) { + + if (!umount_root(impl)) { LOGE("Failed to umount root"); - write_uint8_t(writer, (uint8_t)umount_state); + write_uint8_t(writer, false); close(writer); @@ -793,22 +774,21 @@ int save_mns_fd(int pid, enum MountNamespaceState mns_state, struct root_impl im } } - uint32_t mypid = 0; - while (mypid != (uint32_t)getpid()) { - write_uint8_t(writer, (uint8_t)umount_state); - usleep(50); - read_uint32_t(reader, &mypid); - } + write_uint8_t(writer, true); + + /* INFO: Just a delay for the original process to open ns mnt */ + uint8_t has_opened = 0; + read_uint8_t(reader, &has_opened); close(writer); _exit(0); } - enum mns_umount_state umount_state = (enum mns_umount_state)0; - read_uint8_t(reader, (uint8_t *)&umount_state); + bool has_succeeded = true; + read_uint8_t(reader, (uint8_t *)&has_succeeded); - if (umount_state == Error) { + if (!has_succeeded) { LOGE("Failed to unmount root\n"); close(reader); @@ -830,7 +810,7 @@ int save_mns_fd(int pid, enum MountNamespaceState mns_state, struct root_impl im return -1; } - write_uint32_t(writer, (uint32_t)fork_pid); + write_uint8_t(writer, has_succeeded); if (close(reader) == -1) { LOGE("Failed to close reader: %s\n", strerror(errno)); @@ -850,6 +830,42 @@ int save_mns_fd(int pid, enum MountNamespaceState mns_state, struct root_impl im return -1; } + if (impl.impl == Magisk && impl.variant == Kitsune && mns_state == Clean) { + LOGI("[Magisk] Magisk Kitsune detected, will skip cache first."); + + /* INFO: MagiskSU of Kitsune has a special behavior: It is only mounted + once system boots, because of that, we can only cache once + that happens, or else it will clean the mounts, then later + get MagiskSU mounted, resulting in a mount leak. + + SOURCES: + - https://github.com/1q23lyc45/KitsuneMagisk/blob/8562a0b2ad142d21566c1ea41690ad64108ca14c/native/src/core/bootstages.cpp#L359 + */ + char boot_completed[2]; + get_property("sys.boot_completed", boot_completed); + + if (boot_completed[0] == '1') { + LOGI("[Magisk] Appropriate mns found, caching clean namespace fd."); + + clean_namespace_fd = ns_fd; + } + + /* BUG: For the case where it hasn't booted yet, we will need to + keep creating mns that will be left behind, the issue is: + they are not close'd. This is a problem, because we will + have a leak of fds, although only from the period of booting. + + When trying to close the ns_fd from the libzygisk.so, fdsan + will complain as it is owned by RandomAccessFile, and if + we close from ReZygiskd, system will refuse to boot for + some reason, even if we wait for setns in libzygisk.so, + and that issue is related to setns, as it only happens + with it. + */ + + return ns_fd; + } + if (mns_state == Clean) clean_namespace_fd = ns_fd; else if (mns_state == Mounted) mounted_namespace_fd = ns_fd; diff --git a/zygiskd/src/zygiskd.c b/zygiskd/src/zygiskd.c index 47dce5d..4132d87 100644 --- a/zygiskd/src/zygiskd.c +++ b/zygiskd/src/zygiskd.c @@ -492,7 +492,7 @@ void zygiskd_start(char *restrict argv[]) { size_t modules_len = context.len; ret = write_size_t(client_fd, modules_len); ASSURE_SIZE_WRITE_BREAK("GetInfo", "modules_len", ret, sizeof(modules_len)); - + for (size_t i = 0; i < modules_len; i++) { ret = write_string(client_fd, context.modules[i].name); if (ret == -1) { @@ -639,25 +639,24 @@ void zygiskd_start(char *restrict argv[]) { ASSURE_SIZE_READ_BREAK("UpdateMountNamespace", "mns_state", ret, sizeof(mns_state)); uint32_t our_pid = (uint32_t)getpid(); - ret = write_uint32_t(client_fd, (uint32_t)our_pid); + ret = write_uint32_t(client_fd, our_pid); ASSURE_SIZE_WRITE_BREAK("UpdateMountNamespace", "our_pid", ret, sizeof(our_pid)); - if ((enum MountNamespaceState)mns_state == Clean) { + if ((enum MountNamespaceState)mns_state == Clean) save_mns_fd(pid, Mounted, impl); - } - int clean_namespace_fd = save_mns_fd(pid, (enum MountNamespaceState)mns_state, impl); - if (clean_namespace_fd == -1) { + int ns_fd = save_mns_fd(pid, (enum MountNamespaceState)mns_state, impl); + if (ns_fd == -1) { LOGE("Failed to save mount namespace fd for pid %d: %s\n", pid, strerror(errno)); ret = write_uint32_t(client_fd, (uint32_t)0); - ASSURE_SIZE_WRITE_BREAK("UpdateMountNamespace", "clean_namespace_fd", ret, sizeof(clean_namespace_fd)); + ASSURE_SIZE_WRITE_BREAK("UpdateMountNamespace", "ns_fd", ret, sizeof(ns_fd)); break; } - ret = write_uint32_t(client_fd, (uint32_t)clean_namespace_fd); - ASSURE_SIZE_WRITE_BREAK("UpdateMountNamespace", "clean_namespace_fd", ret, sizeof(clean_namespace_fd)); + ret = write_uint32_t(client_fd, (uint32_t)ns_fd); + ASSURE_SIZE_WRITE_BREAK("UpdateMountNamespace", "ns_fd", ret, sizeof(ns_fd)); break; }