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
DynamoRIO · Jun 14, 2022 · 9f5f753 · 9f5f753
1 parent 613cfae
commit 9f5f753
Show file tree

Hide file tree

Showing 8 changed files with 96 additions and 38 deletions.
diff --git 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
@@ -0,0 +1,5 @@
+Unable to open pagemap for physical addresses: check privileges.
+Output format:
+<record#>: T<tid> <record details>
+------------------------------------------------------------
+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
@@ -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
@@ -35,6 +35,8 @@
 #    include <unistd.h>
 #    include <sys/stat.h>
 #    include <fcntl.h>
+#    include <linux/capability.h>
+#    include <fstream>
 #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<bool> 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<void *>(ppage == 0 ? ZERO_ADDR_PAYLOAD : ppage));

diff --git a/clients/drcachesim/tracer/physaddr.h b/clients/drcachesim/tracer/physaddr.h
@@ -36,8 +36,7 @@
 #ifndef _PHYSADDR_H_
 #define _PHYSADDR_H_ 1
 
-#include <fstream>
-#include <unordered_map>
+#include <atomic>
 #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<bool> has_privileges_;
 #endif
 };
 

diff --git 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<bool> 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
@@ -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
@@ -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}")