From: Tomasz Swierczek Date: Wed, 5 Jul 2023 08:28:57 +0000 (+0200) Subject: Add additional debug information in prepare_app X-Git-Tag: accepted/tizen/unified/20230710.013127~3 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=b6bb26837f33ff3447b153232eeee4fb0d78cca6;p=platform%2Fcore%2Fsecurity%2Fsecurity-manager.git Add additional debug information in prepare_app In case a thread is not receiving signal to change its Smack label & capabilities, additional debugging information is required to check what has happened. Printing the debugging information is followed by an explicit abort() so that the app candidate process can be debugged to know what was happening inside each of the threads. Statuses available to inspect each thread: * thread dead during attempt to send signal -> NOT an issue, but info is printed with TID as it can be useful * thread not synced -> thread was on the list when signal sending attempt was made, but didn't seem to have received signal * thread received signal -> thread did receive signal, but failed at setting Smack label * thread changed Smack label -> it did receive signal, did change Smack label but cannot change caps * thread fully synced -> all went well - information is printed just to help debugging Dump of status of each thread like above is printed ONLY if offending thread is found that didn't seem to process signal handler correctly. Change-Id: Ia1a560fb4baffadc354a403e60d1ab81d8828c42 --- diff --git a/src/client/client-security-manager.cpp b/src/client/client-security-manager.cpp index 9df9d91e..8c5dded6 100644 --- a/src/client/client-security-manager.cpp +++ b/src/client/client-security-manager.cpp @@ -93,6 +93,21 @@ static const std::string *g_p_app_label; static std::atomic g_threads_count; static cap_t g_cap; +// needed to keep track which thread did receive signal and which did not +// additionally keeps information about stage to which each thread got +#define TID_STATUS_NOT_SYNCED 0 +#define TID_STATUS_RECEIVED_SIGNAL 1 +#define TID_STATUS_SETUP_SMACK 2 +#define TID_STATUS_FULLY_SYNCED 3 +#define TID_STATUS_DEAD 4 + +struct TidStatus { + int tid; + int status; +}; + +static TidStatus *g_tid_status; + #define MAX_SIG_WAIT_TIME 1000 // Hackish, based on glibc's definition in sysdeps/unix/sysv/linux/nptl-signals.h @@ -577,9 +592,8 @@ inline static int sigaction(int signum, const struct sigaction *act, struct siga } // namespace Syscall -inline static int label_for_self_internal() +inline static int label_for_self_internal(int tid) { - const auto tid = Syscall::gettid(); char threadSelf[sizeof "/proc//attr/current" + 1 + std::numeric_limits::digits10]; sprintf(threadSelf, "/proc/%u/attr/current", unsigned(tid)); int fd = open(threadSelf, O_WRONLY); @@ -594,6 +608,7 @@ inline static int label_for_self_internal() static inline int security_manager_sync_threads_internal(const std::string &app_label, std::vector &synced_tids) { static_assert(ATOMIC_INT_LOCK_FREE == 2, "std::atomic is not always lock free"); + g_tid_status = NULL; FS::FileNameVector files = FS::getSubDirectoriesFromDirectory("/proc/self/task", true); @@ -609,12 +624,22 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ LogDebug("number of threads to sync: " << files.size() - 1); - const pid_t cur_tid = Syscall::gettid(); + const int cur_tid = Syscall::gettid(); synced_tids.push_back(cur_tid); if (files.size() > 1) { const pid_t cur_pid = getpid(); g_threads_count = files.size() - 1; + g_tid_status = new TidStatus[g_threads_count]; + if (!g_tid_status) { + LogError("Cannot allocate g_tid_status"); + return SECURITY_MANAGER_ERROR_MEMORY; + } else { + for (int i = 0; i < g_threads_count; ++i) { + g_tid_status[i].tid = 0; // to be sure its not yet any actual TID + g_tid_status[i].status = TID_STATUS_NOT_SYNCED; + } + } struct sigaction act; struct sigaction old; @@ -624,16 +649,24 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ act.sa_flags = SA_RESTART; act.sa_handler = [](int signo) { (void)signo; - std::atomic_thread_fence(std::memory_order_acquire); + int tid = Syscall::gettid(); + int i = 0; + for (; i < g_threads_count; ++i) { + if (g_tid_status[i].tid == tid) { + break; + } + } + g_tid_status[i].status = TID_STATUS_RECEIVED_SIGNAL; if (g_p_app_label) - if (label_for_self_internal() != 0) + if (label_for_self_internal(tid) != 0) return; - + g_tid_status[i].status = TID_STATUS_SETUP_SMACK; if (cap_set_proc(g_cap)) return; + g_tid_status[i].status = TID_STATUS_FULLY_SYNCED; g_threads_count--; }; @@ -645,17 +678,21 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ std::atomic_thread_fence(std::memory_order_release); int threads_gone = 0; + int tid_index = 0; for (auto const &e : files) { const int tid = atoi(e.c_str()); - if (tid == static_cast(cur_tid)) + if (tid == cur_tid) continue; synced_tids.push_back(tid); // this will not add current TID (but its already added) + g_tid_status[tid_index++].tid = tid; + if (Syscall::tgkill(cur_pid, tid, SIGSETXID) < 0) { const auto err = errno; - if (ESRCH == err) // thread already gone + if (ESRCH == err) { // thread already gone threads_gone++; - else + g_tid_status[tid_index-1].status = TID_STATUS_DEAD; + } else LogWithErrno(err, "tgkill()"); continue; @@ -672,11 +709,36 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ if (g_threads_count) { LogError("Not all threads synchronized: threads left: " << g_threads_count); - goto err; + for (int i = 0; i < g_threads_count; ++i) { + switch (g_tid_status[i].status) { + case TID_STATUS_DEAD: + /* this is totally fine */ + LogError("Thread " << g_tid_status[i].tid << " died during attempt to send signal - info can be useful, but this is OK"); + break; + case TID_STATUS_NOT_SYNCED: + LogError("Thread " << g_tid_status[i].tid << " didn't receive the signal, serious ERROR!!!"); + break; + case TID_STATUS_RECEIVED_SIGNAL: + LogError("Thread " << g_tid_status[i].tid << " received signal but failed at Smack label change, serious ERROR!!!"); + break; + case TID_STATUS_SETUP_SMACK: + LogError("Thread " << g_tid_status[i].tid << " received signal, changed Smack label, couldn't change caps, serious ERROR!!!"); + break; + case TID_STATUS_FULLY_SYNCED: + /* this is totally fine */ + LogError("Thread " << g_tid_status[i].tid << " received signal and was fully synchronized - info can be useful, but this is OK"); + break; + default: + LogError("Thread " << g_tid_status[i].tid << " has strange status: " << g_tid_status[i].status << " this is an ERROR!!!"); + break; + } + } + LogError("Aborting - please check coredump and check offending threads!"); + abort(); // needed to debug the offending thread(s) } } - if (g_p_app_label && label_for_self_internal() != 0) { + if (g_p_app_label && label_for_self_internal(cur_tid) != 0) { LogError("label_for_self_internal failed"); goto err; } @@ -687,11 +749,15 @@ static inline int security_manager_sync_threads_internal(const std::string &app_ } cap_free(g_cap); + if (g_tid_status) + delete [] g_tid_status; return SECURITY_MANAGER_SUCCESS; err: cap_free(g_cap); + if (g_tid_status) + delete [] g_tid_status; return SECURITY_MANAGER_ERROR_UNKNOWN; }