From 9f5f7538f9d837d75ce93c4e2173582fedb6c0b6 Mon Sep 17 00:00:00 2001 From: Derek Bruening Date: Tue, 14 Jun 2022 10:15:01 -0400 Subject: [PATCH] i#4014 dr$sim phys: Check privileges for pagemap Since the kernel lets an unprivileged user read the pagemap file but supplies 0 values for the physical pages (and 0 is a possible legitimate value), we add an explicit check for privileges and fail up front if physical addresses are requested but not available. This is a change in behavior where before execution would continue with a warning. Adds a test of missing privileges. Fixes a sentinel issue where a 0 physical page was stored as 1 in the table: but 1 is a possible valid number as well, so -1 is used instead. Issue: #4014 --- clients/drcachesim/reader/ipc_reader.cpp | 5 +- .../tests/offline-phys_nopriv.templatex | 5 ++ clients/drcachesim/tests/phys.templatex | 8 +-- clients/drcachesim/tracer/physaddr.cpp | 57 ++++++++++++++++--- clients/drcachesim/tracer/physaddr.h | 13 ++++- clients/drcachesim/tracer/tracer.cpp | 27 ++++----- suite/tests/CMakeLists.txt | 12 +++- suite/tests/runmulti.cmake | 7 ++- 8 files changed, 96 insertions(+), 38 deletions(-) create mode 100644 clients/drcachesim/tests/offline-phys_nopriv.templatex diff --git a/clients/drcachesim/reader/ipc_reader.cpp b/clients/drcachesim/reader/ipc_reader.cpp index 30e1258177d..61341cd86e9 100644 --- a/clients/drcachesim/reader/ipc_reader.cpp +++ b/clients/drcachesim/reader/ipc_reader.cpp @@ -1,5 +1,5 @@ /* ********************************************************** - * Copyright (c) 2015-2020 Google, Inc. All rights reserved. + * Copyright (c) 2015-2022 Google, Inc. All rights reserved. * **********************************************************/ /* @@ -96,6 +96,9 @@ ipc_reader_t::read_next_entry() if (cur_buf_ >= end_buf_) { ssize_t sz = pipe_.read(buf_, sizeof(buf_)); // blocking read if (sz < 0 || sz % sizeof(*end_buf_) != 0) { + // If called again at eof, do not return the footer: return an error. + if (at_eof_) + return nullptr; // We aren't able to easily distinguish truncation from a clean // end (we could at least ensure the prior entry was a thread exit // I suppose). diff --git a/clients/drcachesim/tests/offline-phys_nopriv.templatex b/clients/drcachesim/tests/offline-phys_nopriv.templatex new file mode 100644 index 00000000000..b5eef1274ae --- /dev/null +++ b/clients/drcachesim/tests/offline-phys_nopriv.templatex @@ -0,0 +1,5 @@ +Unable to open pagemap for physical addresses: check privileges. +Output format: +: T +------------------------------------------------------------ +ERROR: failed to initialize analyzer: Failed to create analysis tool: Tool failed to initialize: Failed to load binaries: Failed to parse module file diff --git a/clients/drcachesim/tests/phys.templatex b/clients/drcachesim/tests/phys.templatex index bab9c209ec7..e04b00697cf 100644 --- a/clients/drcachesim/tests/phys.templatex +++ b/clients/drcachesim/tests/phys.templatex @@ -3,8 +3,8 @@ Cache simulation results: Core #0 \(1 thread\(s\)\) L1I stats: - Hits: *[0-9]*[,\.]?... - Misses: *[0-9]..? + Hits: *[0-9,\.]* + Misses: *[0-9,\.]* Compulsory misses: *[0-9,\.]* Invalidations: *0 .* Miss rate: [0-3][,\.]..% @@ -18,8 +18,8 @@ Core #1 \(0 thread\(s\)\) Core #2 \(0 thread\(s\)\) Core #3 \(0 thread\(s\)\) LL stats: - Hits: *[0-9]..? - Misses: *[0-9]*[,\.]?..?.? + Hits: *[0-9,\.]* + Misses: *[0-9,\.]* Compulsory misses: *[0-9,\.]* Invalidations: *0 .* Local miss rate: *[0-9,.]*% diff --git a/clients/drcachesim/tracer/physaddr.cpp b/clients/drcachesim/tracer/physaddr.cpp index 41e6b176f57..fec8131e429 100644 --- a/clients/drcachesim/tracer/physaddr.cpp +++ b/clients/drcachesim/tracer/physaddr.cpp @@ -35,6 +35,8 @@ # include # include # include +# include +# include #endif #include "physaddr.h" #include "../common/options.h" @@ -54,9 +56,10 @@ # define PAGEMAP_VALID 0x8000000000000000 # define PAGEMAP_SWAP 0x4000000000000000 # define PAGEMAP_PFN 0x007fffffffffffff -static const addr_t PAGE_INVALID = (addr_t)-1; #endif +std::atomic physaddr_t::has_privileges_; + physaddr_t::physaddr_t() #ifdef LINUX : page_size_(dr_page_size()) @@ -83,19 +86,57 @@ physaddr_t::physaddr_t() physaddr_t::~physaddr_t() { #ifdef LINUX - NOTIFY(1, - "physaddr: hit cache: " UINT64_FORMAT_STRING - ", hit table " UINT64_FORMAT_STRING ", miss " UINT64_FORMAT_STRING "\n", - num_hit_cache_, num_hit_table_, num_miss_); + if (num_miss_ > 0) { + NOTIFY(1, + "physaddr: hit cache: " UINT64_FORMAT_STRING + ", hit table " UINT64_FORMAT_STRING ", miss " UINT64_FORMAT_STRING "\n", + num_hit_cache_, num_hit_table_, num_miss_); + } if (v2p_ != nullptr) dr_hashtable_destroy(dr_get_current_drcontext(), v2p_); #endif } +bool +physaddr_t::global_init() +{ + // This is invoked at process init time, so we can use heap in C++ classes + // without affecting statically-linked dr$sim. + + // We need CAP_SYS_ADMIN to get valid data out of /proc/self/pagemap + // (the kernel lets us read but just feeds us zeroes otherwise). + constexpr int MAX_STATUS_FNAME = 64; + char statf[MAX_STATUS_FNAME]; + dr_snprintf(statf, BUFFER_SIZE_ELEMENTS(statf), "/proc/%d/status", getpid()); + NULL_TERMINATE_BUFFER(statf); + std::ifstream file(statf); + std::string line; + const std::string want("CapEff"); + while (std::getline(file, line)) { + if (line.compare(0, want.size(), want) != 0) + continue; + std::istringstream ss(line); + std::string unused; + uint64_t bits; + if (ss >> unused >> std::hex >> bits) { + if (TESTALL(1 << CAP_SYS_ADMIN, bits)) { + has_privileges_ = true; + NOTIFY(1, "Has CAP_SYS_ADMIN\n"); + } else + NOTIFY(1, "Does NOT have CAP_SYS_ADMIN\n"); + } + } + DR_ASSERT(std::atomic_is_lock_free(&has_privileges_)); + return has_privileges_; +} + bool physaddr_t::init() { #ifdef LINUX + if (!has_privileges_) + return false; + // Some threads may not do much, so start out small. constexpr int V2P_INITIAL_BITS = 9; // The hashtable lookup performance is important. @@ -205,11 +246,9 @@ physaddr_t::virtual2physical(void *drcontext, addr_t virt, OUT addr_t *phys, NOTIFY(1, "v2p failure: entry is invalid for %p\n", vpage); return false; } - // TODO i#4014: On recent kernels unprivileged reads succeed but are passed - // a 0 PFN. Since that could be valid, we need to check our capabilities - // to decide. Until then, we're currently returning 0 for every unprivileged query. - // Store 0 as a sentinel since 0 means no entry. addr_t ppage = (addr_t)((entry & PAGEMAP_PFN) << page_bits_); + // Despite the kernel handing out a 0 PFN for unprivileged reads, 0 is a valid + // possible PFN. // Store 0 as a sentinel since 0 means no entry. dr_hashtable_add(drcontext, v2p_, vpage, reinterpret_cast(ppage == 0 ? ZERO_ADDR_PAYLOAD : ppage)); diff --git a/clients/drcachesim/tracer/physaddr.h b/clients/drcachesim/tracer/physaddr.h index 23c95fb8685..cef35310466 100644 --- a/clients/drcachesim/tracer/physaddr.h +++ b/clients/drcachesim/tracer/physaddr.h @@ -36,8 +36,7 @@ #ifndef _PHYSADDR_H_ #define _PHYSADDR_H_ 1 -#include -#include +#include #include "dr_api.h" #include "hashtable.h" #include "../common/trace_entry.h" @@ -61,6 +60,12 @@ class physaddr_t { virtual2physical(void *drcontext, addr_t virt, OUT addr_t *phys, OUT bool *from_cache = nullptr); + // This must be called once prior to any instance variables. + // (If this class weren't used in a DR client context we could use a C++ + // mutex or pthread do-once but those are not safe here.) + static bool + global_init(); + private: #ifdef LINUX inline addr_t @@ -93,13 +98,15 @@ class physaddr_t { // The drcontainers hashtable is too slow due to the extra dereferences: // we need an open-addressed table. void *v2p_; + static constexpr addr_t PAGE_INVALID = (addr_t)-1; // With hashtable_t nullptr is how non-existence is shown, so we store // an actual 0 address (can happen for physical) as this sentinel. - static constexpr addr_t ZERO_ADDR_PAYLOAD = 1; + static constexpr addr_t ZERO_ADDR_PAYLOAD = PAGE_INVALID; unsigned int count_; uint64_t num_hit_cache_; uint64_t num_hit_table_; uint64_t num_miss_; + static std::atomic has_privileges_; #endif }; diff --git a/clients/drcachesim/tracer/tracer.cpp b/clients/drcachesim/tracer/tracer.cpp index 76e568da75d..066a12825c6 100644 --- a/clients/drcachesim/tracer/tracer.cpp +++ b/clients/drcachesim/tracer/tracer.cpp @@ -195,9 +195,6 @@ static uint64 num_v2p_writeouts; static uint64 num_phys_markers; static volatile bool exited_process; -/* virtual to physical translation */ -static std::atomic have_phys; - static drmgr_priority_t pri_pre_bbdup = { sizeof(drmgr_priority_t), DRMGR_PRIORITY_NAME_MEMTRACE, NULL, NULL, DRMGR_PRIORITY_APP2APP_DRBBDUP - 1 }; @@ -980,14 +977,15 @@ output_buffer(void *drcontext, per_thread_t *data, byte *buf_base, byte *buf_ptr return current_num_refs; } -// Should be called only for have_phys and -use_physical. +// Should be called only -use_physical. // Returns the byte count to skip in the trace buffer (due to shifting some headers // to the v2p buffer). static size_t process_buffer_for_physaddr(void *drcontext, per_thread_t *data, size_t header_size, byte *buf_ptr) { - ASSERT(have_phys, "Caller must check for use_physical being enabled"); + ASSERT(op_use_physical.get_value(), + "Caller must check for use_physical being enabled"); byte *v2p_ptr = data->v2p_buf; size_t skip = 0; bool emitted = false; @@ -1196,7 +1194,7 @@ memtrace(void *drcontext, bool skip_size_cap) } } size_t skip = 0; - if (have_phys && op_use_physical.get_value()) { + if (op_use_physical.get_value()) { skip = process_buffer_for_physaddr(drcontext, data, header_size, buf_ptr); } current_num_refs += @@ -1555,7 +1553,7 @@ instrument_delay_instrs(void *drcontext, void *tag, instrlist_t *ilist, user_dat // Instrument to add a full instr entry for the first instr. adjust = instru->instrument_instr(drcontext, tag, &ud->instru_field, ilist, where, reg_ptr, adjust, ud->delay_instrs[0]); - if (have_phys && op_use_physical.get_value()) { + if (op_use_physical.get_value()) { // No instr bundle if physical-2-virtual since instr bundle may // cross page bundary. int i; @@ -2767,9 +2765,9 @@ init_thread_in_process(void *drcontext) if (op_use_physical.get_value()) { if (!data->physaddr.init()) { - have_phys = false; - NOTIFY(0, "Unable to open pagemap: using virtual addresses for thread T%d.\n", - dr_get_thread_id(drcontext)); + FATAL("Unable to open pagemap for physical addresses in thread T%d: check " + "privileges.\n", + dr_get_thread_id(drcontext)); } } @@ -3163,7 +3161,6 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[]) DR_ASSERT(std::atomic_is_lock_free(&reached_trace_after_instrs)); DR_ASSERT(std::atomic_is_lock_free(&tracing_disabled)); DR_ASSERT(std::atomic_is_lock_free(&tracing_window)); - DR_ASSERT(std::atomic_is_lock_free(&have_phys)); drreg_init_and_fill_vector(&scratch_reserve_vec, true); #ifdef X86 @@ -3173,11 +3170,6 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[]) } #endif - if (op_use_physical.get_value()) { - // This will be set to false if any thread fails. - have_phys = true; - } - if (op_offline.get_value()) { void *placement; if (!init_offline_dir()) { @@ -3323,6 +3315,9 @@ drmemtrace_client_main(client_id_t id, int argc, const char *argv[]) /* We need the same is-buffer-zero checks in the instrumentation. */ thread_filtering_enabled = true; } + + if (op_use_physical.get_value() && !physaddr_t::global_init()) + FATAL("Unable to open pagemap for physical addresses: check privileges.\n"); } /* To support statically linked multiple clients, we add drmemtrace_client_main diff --git a/suite/tests/CMakeLists.txt b/suite/tests/CMakeLists.txt index 056d0a42d9f..a2190308906 100644 --- a/suite/tests/CMakeLists.txt +++ b/suite/tests/CMakeLists.txt @@ -1439,6 +1439,7 @@ function(torun test key source native standalone_dr dr_ops exe_ops added_out pas -D postcmd=${${key}_postcmd} -D postcmd2=${${key}_postcmd2} -D postcmd3=${${key}_postcmd3} + -D failok=${${key}_failok} -D cmp=${CMAKE_CURRENT_BINARY_DIR}/${expectbase}.expect -D code=${${key}_code} -D capture=${${key}_runcmp_capture} @@ -3348,8 +3349,12 @@ if (BUILD_CLIENTS) # would need an analyzer to go examine addresses, or to use the view tool. # For now these are just sanity tests that flipping on the option doesn't # cause outright failure. - torunonly_drcachesim(phys ${ci_shared_app} "-use_physical" "") - torunonly_drcachesim(phys-threads client.annotation-concurrency "-use_physical" + set(tool.drcachesim.phys_SUDO_sudo ON) + set(tool.drcachesim.phys_SUDO_expectbase "phys") + torunonly_drcachesim(phys_SUDO ${ci_shared_app} "-use_physical" "") + set(tool.drcachesim.phys-threads_SUDO_sudo ON) + set(tool.drcachesim.phys-threads_SUDO_expectbase "phys-threads") + torunonly_drcachesim(phys-threads_SUDO client.annotation-concurrency "-use_physical" "${annotation_test_args_shorter}") endif () @@ -3756,6 +3761,9 @@ if (BUILD_CLIENTS) set(tool.drcacheoff.phys_SUDO_expectbase "offline-phys") torunonly_drcacheoff(phys_SUDO allasm_x86_64 "-use_physical" "@${test_mode_flag}@-simulator_type@view" "") + set(tool.drcacheoff.phys_nopriv_failok ON) + torunonly_drcacheoff(phys_nopriv allasm_x86_64 + "-use_physical" "@${test_mode_flag}@-simulator_type@view" "") endif () if (UNIX AND ZLIB_FOUND) diff --git a/suite/tests/runmulti.cmake b/suite/tests/runmulti.cmake index eb551590d48..ea135791f6c 100644 --- a/suite/tests/runmulti.cmake +++ b/suite/tests/runmulti.cmake @@ -1,5 +1,5 @@ # ********************************************************** -# Copyright (c) 2015-2018 Google, Inc. All rights reserved. +# Copyright (c) 2015-2022 Google, Inc. All rights reserved. # ********************************************************** # Redistribution and use in source and binary forms, with or without @@ -35,6 +35,7 @@ # * postcmd = post processing command to run # * postcmdN (for N=2+) = additional post processing commands to run # * cmp = the file containing the expected output +# * failok = if ON then non-zero exit codes are ignored for cmd # # A "*" in any command line will be glob-expanded right before running. # If the command starts with "foreach@", instead of passing the glob-expansion @@ -102,9 +103,9 @@ macro(process_cmdline line skip_empty err_and_out) RESULT_VARIABLE cmd_result ERROR_VARIABLE cmd_err OUTPUT_VARIABLE cmd_out) - if (cmd_result) + if (cmd_result AND NOT failok) message(FATAL_ERROR "*** ${line} failed (${cmd_result}): ${cmd_err}***\n") - endif (cmd_result) + endif () endif () endif () set(${err_and_out} "${${err_and_out}}${cmd_err}${cmd_out}")