[compiler-rt][rtsan] ptrace interception on Linux.

[devnexen]

No description provided.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[llvmbot]

@llvm/pr-subscribers-compiler-rt-sanitizer

Author: David CARLIER (devnexen)

Changes

---

Full diff: https://github.com/llvm/llvm-project/pull/123941.diff

2 Files Affected:

• (modified) compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp (+38)
• (modified) compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp (+11)

diff --git a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
index 71938d3edba38d..64e3fab8200efc 100644
--- a/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/rtsan_interceptors_posix.cpp
@@ -41,6 +41,10 @@ void OSSpinLockLock(volatile OSSpinLock *__lock);
 #include <malloc.h>
 #endif
 
+#if SANITIZER_INTERCEPT_PTRACE
+#include <sys/ptrace.h>
+#endif
+
 #include <fcntl.h>
 #include <poll.h>
 #include <pthread.h>
@@ -1161,6 +1165,39 @@ INTERCEPTOR(ssize_t, process_vm_writev, pid_t pid,
 #define RTSAN_MAYBE_INTERCEPT_PROCESS_VM_WRITEV
 #endif
 
+#if SANITIZER_INTERCEPT_PTRACE
+#if SANITIZER_MUSL
+INTERCEPTOR(long, ptrace, int request, ...) {
+#else
+INTERCEPTOR(long, ptrace, enum __ptrace_request request, ...) {
+#endif // SANITIZER_MUSL
+  __rtsan_notify_intercepted_call("ptrace");
+  va_list args;
+
+  // A handful of ptrace requests need an additional argument on Linux/sparc
+  // (e.g. PTRACE_READDATA) but we only intercept the standard calls at the
+  // moment. We might need to rework all if rtsan is supported on BSD,
+  // interfaces differ vastly, data is read in word size on Linux vs large
+  // chunks on freebsd and so on ...
+  va_start(args, request);
+  pid_t pid = va_arg(args, pid_t);
+
+  // addr and data types depend on the request, either of these are ignored in
+  // some cases too. using intptr_t to be able to hold accepted ptrace types.
+  using arg_type = intptr_t;
+  static_assert(sizeof(arg_type) >= sizeof(struct ptrace_seekinfo_args *));
+  static_assert(sizeof(arg_type) >= sizeof(int));
+  arg_type addr = va_arg(args, arg_type);
+  arg_type priv = va_arg(args, arg_type);
+  va_end(args);
+
+  return REAL(ptrace)(request, pid, addr, priv);
+}
+#define RTSAN_MAYBE_INTERCEPT_PTRACE INTERCEPT_FUNCTION(ptrace)
+#else
+#define RTSAN_MAYBE_INTERCEPT_PTRACE
+#endif
+
 // TODO: the `wait` family of functions is an oddity. In testing, if you
 // intercept them, Darwin seemingly ignores them, and linux never returns from
 // the test. Revisit this in the future, but hopefully intercepting fork/exec is
@@ -1347,6 +1384,7 @@ void __rtsan::InitializeInterceptors() {
 
   RTSAN_MAYBE_INTERCEPT_PROCESS_VM_READV;
   RTSAN_MAYBE_INTERCEPT_PROCESS_VM_WRITEV;
+  RTSAN_MAYBE_INTERCEPT_PTRACE;
 
   INTERCEPT_FUNCTION(syscall);
 }
diff --git a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
index 0a59ae0ea92548..8de6c3f9599368 100644
--- a/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
+++ b/compiler-rt/lib/rtsan/tests/rtsan_test_interceptors_posix.cpp
@@ -43,6 +43,9 @@
 #include <poll.h>
 #include <pthread.h>
 #include <stdio.h>
+#if SANITIZER_INTERCEPT_PTRACE
+#include <sys/ptrace.h>
+#endif
 #include <sys/ioctl.h>
 #include <sys/mman.h>
 #include <sys/socket.h>
@@ -748,6 +751,14 @@ TEST(TestRtsanInterceptors, ProcessVmWritevDiesWhenRealtime) {
 }
 #endif
 
+#if SANITIZER_INTERCEPT_PTRACE
+TEST(TestRtsanInterceptors, PtraceWhenRealtime) {
+  auto Func = []() { ptrace(PTRACE_PEEKUSER, -1, 0, nullptr); };
+  ExpectRealtimeDeath(Func, "ptrace");
+  ExpectNonRealtimeSurvival(Func);
+}
+#endif
+
 class RtsanDirectoryTest : public ::testing::Test {
 protected:
   void SetUp() override {

[cjappl]

I think this is another one like the scheduling ones - this call is used to debug, do we intercept it or assume that someone using it knows what they are doing? An alternative read could be "People should be notified if they accidentally leave ptrace in, and they will not be using it alongside rtsan".

@davidtrevelyan do you think we should intercept this call? I'm on the fence

[davidtrevelyan]

@cjappl i lean towards intercepting - remembering that our users care about real-time-unsafe calls from within dependencies as well as from within their own code. I'd say that we have easy-enough escape hatches for people who really want to escape to make the cost of missing a ptrace enough to outweigh the cost of disabling when you really want it :)

[cjappl]

Just putting a hold on this review so we can chat about my comment some more.

[cjappl]

I think my concern with merging this is it's complexity.

I agree this call is real-time unsafe, but with the va_args and all this wrangling of them we have to ask "What are the odds this detects a real-time safety violation in user code?" vs "What if we inadvertently mess up and break someones code in an insidious way?"

This call and prctl (#124880) both are on the side of the complexity outweighing the benefit for me. I think we are more likely to introduce a bug than detect something in users code.

The more common a call is, and the easier to intercept it, the more we should lean towards intercepting.

That's my 2c, @davidtrevelyan any thoughts?

[davidtrevelyan]

I don't feel like I have enough knowledge about the common usage of prctl and ptrace to be able to provide meaningful insight here. @devnexen - what was the main motivation for introducing these interceptors from your perspective? In your experience, are these functions often called directly by developers? Is there a common route to them through some standard library tooling?

[devnexen]

prctl is often used to alter the current process behavior, e.g. PR_DUMPABLE/PRNAME to name some of the most used, PR*_THP_DISABLE you can typically find it in memory allocators for example. However, ptrace is really only for debugging, e.g. malware analysis or interacting with a debugger ; so it has definitively a less broad usage.

[davidtrevelyan]

Thanks for the extra info @devnexen, and apologies for the delayed response. Revisiting this now (having seen it inactive for a couple of months) I feel it's still not clear whether this interceptor (and the one for prctl #124880 ) is important enough for the real-time programmer to outweigh the risks. I'd propose we close these two PRs for now, and re-open them later if there is renewed demand. @cjappl @devnexen how would you feel about this?

[devnexen]

Regarding all the "controversy" it triggers, I m going to close it at least for now.