From 7ba5030dcceba616927dc7deda3b46ee009de4e4 Mon Sep 17 00:00:00 2001 From: Nikita Ioffe Date: Fri, 27 Nov 2020 18:57:44 +0000 Subject: [PATCH] Fix potential use-after-free bug in reboot Instead of operating on raw pointers, init now uses name of the services as it's primary identifier. Only place that still uses vector is StopServices. In addition, ServiceList::services() function is removed, which should help avoiding similar bugs in the future. Bug: 170315126 Bug: 174335499 Test: adb reboot Test: atest CtsInitTestCases Change-Id: I73ecd7a8c58c2ec3732934c595b7f7db814b7034 Merged-In: I73ecd7a8c58c2ec3732934c595b7f7db814b7034 Ignore-AOSP-First: fixing security vulnerability (cherry picked from commit 8d6ae2dd8aebfd98e5b0968062b2f0c92fa0b0da) --- init/lmkd_service.cpp | 2 +- init/reboot.cpp | 62 +++++++++++++++++-------------- init/service_list.h | 1 - init/test_utils/service_utils.cpp | 2 +- 4 files changed, 36 insertions(+), 31 deletions(-) diff --git a/init/lmkd_service.cpp b/init/lmkd_service.cpp index dd1ab4d61..c982925ad 100644 --- a/init/lmkd_service.cpp +++ b/init/lmkd_service.cpp @@ -79,7 +79,7 @@ static bool UnregisterProcess(pid_t pid) { } static void RegisterServices(pid_t exclude_pid) { - for (const auto& service : ServiceList::GetInstance().services()) { + for (const auto& service : ServiceList::GetInstance()) { auto svc = service.get(); if (svc->oom_score_adjust() != DEFAULT_OOM_SCORE_ADJUST) { // skip if process is excluded or not yet forked (pid==0) diff --git a/init/reboot.cpp b/init/reboot.cpp index 49baf9e68..409f8cb31 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -85,12 +85,11 @@ static bool shutting_down = false; static const std::set kDebuggingServices{"tombstoned", "logd", "adbd", "console"}; -static std::vector GetDebuggingServices(bool only_post_data) { - std::vector ret; - ret.reserve(kDebuggingServices.size()); +static std::set GetPostDataDebuggingServices() { + std::set ret; for (const auto& s : ServiceList::GetInstance()) { - if (kDebuggingServices.count(s->name()) && (!only_post_data || s->is_post_data())) { - ret.push_back(s.get()); + if (kDebuggingServices.count(s->name()) && s->is_post_data()) { + ret.insert(s->name()); } } return ret; @@ -503,13 +502,18 @@ static Result KillZramBackingDevice() { // Stops given services, waits for them to be stopped for |timeout| ms. // If terminate is true, then SIGTERM is sent to services, otherwise SIGKILL is sent. -static void StopServices(const std::vector& services, std::chrono::milliseconds timeout, +// Note that services are stopped in order given by |ServiceList::services_in_shutdown_order| +// function. +static void StopServices(const std::set& services, std::chrono::milliseconds timeout, bool terminate) { LOG(INFO) << "Stopping " << services.size() << " services by sending " << (terminate ? "SIGTERM" : "SIGKILL"); std::vector pids; pids.reserve(services.size()); - for (const auto& s : services) { + for (const auto& s : ServiceList::GetInstance().services_in_shutdown_order()) { + if (services.count(s->name()) == 0) { + continue; + } if (s->pid() > 0) { pids.push_back(s->pid()); } @@ -529,12 +533,12 @@ static void StopServices(const std::vector& services, std::chrono::mil // Like StopServices, but also logs all the services that failed to stop after the provided timeout. // Returns number of violators. -static int StopServicesAndLogViolations(const std::vector& services, +static int StopServicesAndLogViolations(const std::set& services, std::chrono::milliseconds timeout, bool terminate) { StopServices(services, timeout, terminate); int still_running = 0; - for (const auto& s : services) { - if (s->IsRunning()) { + for (const auto& s : ServiceList::GetInstance()) { + if (s->IsRunning() && services.count(s->name())) { LOG(ERROR) << "[service-misbehaving] : service '" << s->name() << "' is still running " << timeout.count() << "ms after receiving " << (terminate ? "SIGTERM" : "SIGKILL"); @@ -620,8 +624,7 @@ static void DoReboot(unsigned int cmd, const std::string& reason, const std::str // watchdogd is a vendor specific component but should be alive to complete shutdown safely. const std::set to_starts{"watchdogd"}; - std::vector stop_first; - stop_first.reserve(ServiceList::GetInstance().services().size()); + std::set stop_first; for (const auto& s : ServiceList::GetInstance()) { if (kDebuggingServices.count(s->name())) { // keep debugging tools until non critical ones are all gone. @@ -639,7 +642,7 @@ static void DoReboot(unsigned int cmd, const std::string& reason, const std::str << "': " << result.error(); } } else { - stop_first.push_back(s.get()); + stop_first.insert(s->name()); } } @@ -702,7 +705,7 @@ static void DoReboot(unsigned int cmd, const std::string& reason, const std::str LOG(INFO) << "vold not running, skipping vold shutdown"; } // logcat stopped here - StopServices(GetDebuggingServices(false /* only_post_data */), 0ms, false /* SIGKILL */); + StopServices(kDebuggingServices, 0ms, false /* SIGKILL */); // 4. sync, try umount, and optionally run fsck for user shutdown { Timer sync_timer; @@ -784,17 +787,17 @@ static Result DoUserspaceReboot() { sub_reason = "resetprop"; return Error() << "Failed to reset sys.powerctl property"; } - std::vector stop_first; + std::set stop_first; // Remember the services that were enabled. We will need to manually enable them again otherwise // triggers like class_start won't restart them. - std::vector were_enabled; - stop_first.reserve(ServiceList::GetInstance().services().size()); + std::set were_enabled; for (const auto& s : ServiceList::GetInstance().services_in_shutdown_order()) { if (s->is_post_data() && !kDebuggingServices.count(s->name())) { - stop_first.push_back(s); + stop_first.insert(s->name()); } + // TODO(ioffe): we should also filter out temporary services here. if (s->is_post_data() && s->IsEnabled()) { - were_enabled.push_back(s); + were_enabled.insert(s->name()); } } { @@ -814,8 +817,8 @@ static Result DoUserspaceReboot() { r > 0) { auto fd = unique_fd(TEMP_FAILURE_RETRY(open(services_file_name.c_str(), flags, 0666))); android::base::WriteStringToFd("Post-data services still running: \n", fd); - for (const auto& s : stop_first) { - if (s->IsRunning()) { + for (const auto& s : ServiceList::GetInstance()) { + if (s->IsRunning() && stop_first.count(s->name())) { android::base::WriteStringToFd(s->name() + "\n", fd); } } @@ -830,13 +833,14 @@ static Result DoUserspaceReboot() { sub_reason = "vold_reset"; return result; } - if (int r = StopServicesAndLogViolations(GetDebuggingServices(true /* only_post_data */), - sigkill_timeout, false /* SIGKILL */); + const auto& debugging_services = GetPostDataDebuggingServices(); + if (int r = StopServicesAndLogViolations(debugging_services, sigkill_timeout, + false /* SIGKILL */); r > 0) { auto fd = unique_fd(TEMP_FAILURE_RETRY(open(services_file_name.c_str(), flags, 0666))); android::base::WriteStringToFd("Debugging services still running: \n", fd); - for (const auto& s : GetDebuggingServices(true)) { - if (s->IsRunning()) { + for (const auto& s : ServiceList::GetInstance()) { + if (s->IsRunning() && debugging_services.count(s->name())) { android::base::WriteStringToFd(s->name() + "\n", fd); } } @@ -866,9 +870,11 @@ static Result DoUserspaceReboot() { return false; }); // Re-enable services - for (const auto& s : were_enabled) { - LOG(INFO) << "Re-enabling service '" << s->name() << "'"; - s->Enable(); + for (const auto& s : ServiceList::GetInstance()) { + if (were_enabled.count(s->name())) { + LOG(INFO) << "Re-enabling service '" << s->name() << "'"; + s->Enable(); + } } ServiceList::GetInstance().ResetState(); LeaveShutdown(); diff --git a/init/service_list.h b/init/service_list.h index 3b9018bc0..555da258a 100644 --- a/init/service_list.h +++ b/init/service_list.h @@ -66,7 +66,6 @@ class ServiceList { auto begin() const { return services_.begin(); } auto end() const { return services_.end(); } - const std::vector>& services() const { return services_; } const std::vector services_in_shutdown_order() const; void MarkPostData(); diff --git a/init/test_utils/service_utils.cpp b/init/test_utils/service_utils.cpp index ae68679bd..6426ed9fd 100644 --- a/init/test_utils/service_utils.cpp +++ b/init/test_utils/service_utils.cpp @@ -44,7 +44,7 @@ android::base::Result GetOnDeviceServiceInterfacesMap() { } ServiceInterfacesMap result; - for (const auto& service : service_list.services()) { + for (const auto& service : service_list) { // Create an entry for all services, including services that may not // have any declared interfaces. result[service->name()] = service->interfaces();