From: Krzysztof Jackiewicz Date: Thu, 5 Dec 2024 16:26:59 +0000 (+0100) Subject: Don't use allocations during signaling X-Git-Tag: accepted/tizen/unified/20241209.090229~1 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=61268c75f03fac2fc048fe1bac5311469225db6c;p=platform%2Fcore%2Fsecurity%2Fsecurity-manager.git Don't use allocations during signaling This is to prevent deadlock in malloc. It may happen if a supplementary thread is interrupted after it took an internal glibc lock in malloc() and it waits in a signal handler for the main thread. The main thread won't be able to allocate memory due to lock taken by supplementary thread. Change-Id: I218075c2c2d6befa8fafb141e0507e64b5b47406 --- diff --git a/src/client/CMakeLists.txt b/src/client/CMakeLists.txt index c8db9388..bea95717 100644 --- a/src/client/CMakeLists.txt +++ b/src/client/CMakeLists.txt @@ -49,7 +49,6 @@ SET(CLIENT_SOURCES ${CLIENT_PATH}/client-common.cpp ${CLIENT_PATH}/client-offline.cpp ${CLIENT_PATH}/client-label-monitor.cpp - ${CLIENT_PATH}/check-proper-drop.cpp ) LINK_DIRECTORIES(${CLIENT_DEP_LIBRARY_DIRS}) diff --git a/src/client/check-proper-drop.cpp b/src/client/check-proper-drop.cpp deleted file mode 100644 index 0f374362..00000000 --- a/src/client/check-proper-drop.cpp +++ /dev/null @@ -1,75 +0,0 @@ -/* - * Copyright (c) 2016-2023 Samsung Electronics Co., Ltd. All rights reserved. - * - * This file is licensed under the terms of MIT License or the Apache License - * Version 2.0 of your choice. See the LICENSE.MIT file for MIT license details. - * See the LICENSE file or the notice below for Apache License Version 2.0 - * details. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ - -/* - * @brief Implementation of proper privilege dropping check utilities, client side - */ - -#include - -#include -#include -#include -#include - -namespace SecurityManager { -namespace CheckProperDrop { - -namespace { - -DECLARE_EXCEPTION_TYPE(SecurityManager::Exception, DropError) - -} // namespace - -std::unordered_set checkThreads(int currentTid) -{ - FS::FileNameVector current_tids; - for (unsigned i = 0; i < 10; ++i) { - try { - current_tids = FS::getSubDirectoriesFromDirectory("/proc/self/task", true); - break; - } catch(const FS::Exception::FileError&) { - if (i == 9) - throw; - - /* - * This may happen if a thread disappears, an entry is removed from /proc and fstatat - * fails. - */ - LogWarning("Reading /proc/self/task/ failed. Retrying."); - } - } - - std::unordered_set result; - for (auto &tid_s : current_tids) { - int tid = atoi(tid_s.c_str()); - - if (tid == currentTid) - continue; - - LogDebug("Thread " << tid_s << " new"); - result.emplace(tid); - } - return result; -} - -} // namespace CheckProperDrop -} // namespace SecurityManager diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index ab2ed3af..18568428 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -102,6 +102,8 @@ static std::atomic g_th_barrier; static std::atomic g_thread_state[MAX_THREAD_COUNT]; static std::atomic g_thread_index[MAX_THREAD_COUNT]; static std::atomic g_managed_tids_num; +static bool g_thread_alive[MAX_THREAD_COUNT]; +static bool g_thread_signaled[MAX_THREAD_COUNT]; static cap_t g_cap; static inline void add_managed_tid(int tid){ @@ -111,6 +113,7 @@ static inline void add_managed_tid(int tid){ } g_thread_state[g_managed_tids_num] = 0; g_thread_index[g_managed_tids_num] = tid; + g_thread_alive[g_managed_tids_num] = true; g_managed_tids_num++; } @@ -132,14 +135,6 @@ static inline void set_tid_state(int tid, int state = 0) { g_thread_state[index] = state; } -static int count_alive_tids_with_state(int status, const std::unordered_set& alive_tids) { - int count = 0; - for (int i = 0; i < g_managed_tids_num; ++i) - if (g_thread_state[i] == status && alive_tids.find(g_thread_index[i]) != alive_tids.end()) - ++ count; - return count; -} - SECURITY_MANAGER_API const char *security_manager_strerror(enum lib_retcode rc) { @@ -645,6 +640,175 @@ static inline void security_manager_pre_check() cap_free(my_caps); } +static bool alive_threads_have_state(int status) noexcept +{ + for (int i = 0; i < g_managed_tids_num; ++i) { + if (!g_thread_alive[i]) + continue; + + if (g_thread_state[i] != status) + return false; + } + return true; +} + +static int get_alive_threads(int own_tid) noexcept +{ + // reset alive status + memset(&g_thread_alive, 0, sizeof(g_thread_alive)); + + auto dir_fd = open("/proc/self/task", O_DIRECTORY | O_CLOEXEC); + if (dir_fd == -1) + return errno; + + char buf[1024]; + struct dirent64 *d; + int bpos = 0; + for(;;) { + ssize_t nread = getdents64(dir_fd, buf, sizeof(buf)); + if (nread == -1) { + close(dir_fd); + return errno; + } + + if (nread == 0) + break; + + for (bpos = 0; bpos < nread; bpos += d->d_reclen) { + d = (struct dirent64 *) (buf + bpos); + + // TODO it's DT_UNKNOWN for some reason +// d_type = *(buf + bpos + d->d_reclen - 1); +// if (d_type != DT_DIR) +// continue; + + if (strcmp(d->d_name, ".") == 0 || strcmp(d->d_name, "..") == 0) + continue; + + char* endptr; + + errno = 0; + long tid = strtol(d->d_name, &endptr, 10); + if (errno != 0) { + close(dir_fd); + return errno; + } + + if (tid == own_tid) + continue; + + // Mark as alive. Append new if necessary. + int i = 0; + for (; i < g_managed_tids_num; ++i) { + if (g_thread_index[i] == tid) { + g_thread_alive[i] = true; + break; + } + } + if (i != g_managed_tids_num) + continue; + + add_managed_tid(tid); + } + } + + close(dir_fd); + return 0; +} + +static int check_threads(int own_tid) noexcept +{ + for (unsigned i = 0; i < 10; ++i) { + auto ret = get_alive_threads(own_tid); + if (ret == 0) + break; + + /* + * This may happen if a thread disappears, an entry is removed from /proc and fstatat + * fails. + */ + if (i == 9) + return ret; + } + + return 0; +} + +static bool no_new_threads() noexcept +{ + for (int i = 0; i < g_managed_tids_num; ++i) { + if (!g_thread_alive[i]) + continue; + + if (!g_thread_signaled[i]) + return false; + } + return true; +} + +static int signal_and_wait_for_handlers(pid_t own_pid, int own_tid) noexcept +{ + // No allocations allowed in this function + int ret = 0; + int time_left = MAX_SIG_WAIT_TIME; + do { + ret = check_threads(own_tid); + if (ret != 0) + break; + + for (int i = 0; i < g_managed_tids_num; ++i) { + if (!g_thread_alive[i]) + break; + + if (!g_thread_signaled[i]) { + g_thread_signaled[i] = true; + int tid = g_thread_index[i]; + + // thread not signaled yet, send signal + if (Syscall::tgkill(own_pid, tid, SIGSETXID) < 0) { + auto err = errno; + if (EAGAIN == err) { // resource temporarily unavailable, trying again + usleep(SLEEP_CONST); // 10 ms + if (Syscall::tgkill(own_pid, tid, SIGSETXID) < 0) { + err = errno; + if (ESRCH == err) { // thread already gone - noop + continue; + } else { + return err; + } + } + } else if (ESRCH == err) { // thread already gone - noop + continue; + } else { + return err; + } + } + } + } + + usleep(SLEEP_CONST); // 10 ms + --time_left; + + // break if number of threads in waiting state equals to number of alive tids minus current one + if (alive_threads_have_state(1)) { + // threads that were read some lines above are all sleeping here - BUT - could have + // spawned new threads between reading list from /proc and checking status with count_alive_tids_with_state() + // to make sure the loop can end, here we read /proc again + ret = check_threads(own_tid); + if (ret != 0) + break; + + if (no_new_threads()) + // here indeed, no new threads are present and they are all waiting in the signal handler + break; + } + + if (time_left == 0) + return ETIME; + } while (1); + return ret; +} + static inline int security_manager_sync_threads_internal(const std::string &app_label) { static_assert(ATOMIC_INT_LOCK_FREE == 2, "std::atomic is not always lock free"); @@ -653,8 +817,6 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ int own_tid = Syscall::gettid(); const pid_t own_pid = getpid(); - std::unordered_set tids_sent_signals = {}; - g_cap = cap_init(); if (!g_cap) { @@ -700,65 +862,12 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ std::atomic_thread_fence(std::memory_order_release); g_th_barrier = 0; - int time_left = MAX_SIG_WAIT_TIME; - std::unordered_set current_tids = {}; - do { - current_tids = CheckProperDrop::checkThreads(own_tid); - for (auto existing_tid : current_tids) { - if (tids_sent_signals.find(existing_tid) == tids_sent_signals.end()) { - tids_sent_signals.insert(existing_tid); - add_managed_tid(existing_tid); - // thread not managed yet, send signal - if (Syscall::tgkill(own_pid, existing_tid, SIGSETXID) < 0) { - auto err = errno; - if (EAGAIN == err) { // resource temporarily unavailable, trying again - LogWarning("Received EAGAIN from tgkill, wait a bit & try again"); - usleep(SLEEP_CONST); // 10 ms - if (Syscall::tgkill(own_pid, existing_tid, SIGSETXID) < 0) { - err = errno; - if (ESRCH == err) { // thread already gone - noop - continue; - } else { - LogWithErrno(err, "tgkill()"); - abort(); - } - } - } else if (ESRCH == err) { // thread already gone - noop - continue; - } else { - LogWithErrno(err, "tgkill()"); - abort(); - } - } - } - } - - usleep(SLEEP_CONST); // 10 ms - --time_left; - - // break if number of threads in waiting state equals to number of alive tids minus current one - int ready_tids = count_alive_tids_with_state(1, current_tids); - if (ready_tids == (int)current_tids.size()) { - // threads that were read some lines above are all sleeping here - BUT - could have - // spawned new threads between reading list from /proc and checking status with count_alive_tids_with_state() - // to make sure the loop can end, here we read /proc again - if(CheckProperDrop::checkThreads(own_tid) == current_tids) - // here indeed, no new threads are present and they are all waiting in the signal handler - break; - } - - if (time_left % 500 == 0) - LogWarning("Still waiting for threads to receive signals... signals received by: " << ready_tids - << ", all tids alive now without current: " - << current_tids.size()); - if (time_left == 0) { - LogError("Too much waiting for receiving signals, aborting! Signals received by: " << ready_tids - << ", out of alive tids without current: " - << current_tids.size()); - abort(); - } - } while (1); + int ret = signal_and_wait_for_handlers(own_pid, own_tid); + if (ret != 0) { + LogError("Error occured during signaling: " << ret); + abort(); + } // here, all TIDs except current one are waiting to start changing attributes // We can assume these TIDs will continue to live (no need to read /proc again), since no logic @@ -766,25 +875,21 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ g_th_barrier++; // this starts the signal handlers - they will proceed once they wake up - int ready_tids = 0; - + bool ready = false; for (int i = MAX_SIG_WAIT_TIME; i > 0; --i) { // break if number of completed tids equals to all of them minus current one! - ready_tids = count_alive_tids_with_state(2, current_tids); - if (ready_tids == (int)current_tids.size()) + + ready = alive_threads_have_state(2); + if (ready) break; usleep(SLEEP_CONST); // 10 ms if (i % 500 == 0) - LogWarning("Still waiting for threads to finalize handlers... completed by: " << ready_tids - << ", all tids managed now: " - << current_tids.size()); + LogWarning("Still waiting for threads to finalize handlers."); } - if (ready_tids != (int)current_tids.size()) { + if (!ready) { // not all TIDs reached this stage, aborting! - LogError("Too much waiting for sig handler completion, aborting! Signals completed by: " << ready_tids - << ", all tids managed now: " - << current_tids.size()); + LogError("Too much waiting for sig handler completion, aborting!"); abort(); } diff --git a/src/common/include/check-proper-drop.h b/src/common/include/check-proper-drop.h index 03e07345..061b664c 100644 --- a/src/common/include/check-proper-drop.h +++ b/src/common/include/check-proper-drop.h @@ -26,7 +26,6 @@ #pragma once #include -#include namespace SecurityManager { namespace CheckProperDrop { @@ -45,10 +44,5 @@ static_assert(N_FLAG_BITS <= 7, "flags too large for int8_t"); // Otherwise bit i set iff NS_NAMES[i] checked. int8_t computeFlags(); - -// CLIENT ONLY -// returns threads TIDS -std::unordered_set checkThreads(int currentTid); - } // namespace CheckProperDrop } // namespace SecurityManager diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 34029cb6..14444a6b 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -126,7 +126,6 @@ SET(SM_TESTS_SOURCES ${PROJECT_SOURCE_DIR}/src/common/filesystem.cpp ${PROJECT_SOURCE_DIR}/src/common/tzplatform-config.cpp ${PROJECT_SOURCE_DIR}/src/common/utils.cpp - ${PROJECT_SOURCE_DIR}/src/client/check-proper-drop.cpp ${GEN_PATH}/db.h ) diff --git a/test/test_misc.cpp b/test/test_misc.cpp index d536b122..c24bbe16 100644 --- a/test/test_misc.cpp +++ b/test/test_misc.cpp @@ -27,7 +27,6 @@ */ #include -#include #include #include #include @@ -211,13 +210,6 @@ POSITIVE_TEST_CASE(T293_possiblyUnterminatedArrayToString) BOOST_REQUIRE_EQUAL("", possiblyUnterminatedArrayToString(a)); } -POSITIVE_TEST_CASE(T294_checkThreads) -{ - pid_t myTid = gettid(); - auto tids = CheckProperDrop::checkThreads(myTid); - BOOST_REQUIRE_MESSAGE(tids.count(myTid) == 0, "CheckProperDrop::checkThreads includes own tid"); -} - POSITIVE_TEST_CASE(T295_GetErrnoString) { std::string str = GetErrnoString(EINVAL);