Fix detection of interfering branches for PATCH_SYSCALL_INSTRUCTION_I…

…S_LAST

The interfering branch check was assuming that the syscall (or rdtsc
instruction) is at the start of the patch region. As a result, it
was incorrectly ignoring interfering branches for
PATCH_SYSCALL_INSTRUCTION_IS_LAST hooks (as well as detecting
interfering branches that actually do not interfere).

CMakeLists.txt

@@ -1454,6 +1454,7 @@ set(TESTS_WITH_PROGRAM
   ptrace_remote_unmap
   x86/rdtsc_loop
   x86/rdtsc_loop2
+  x86/rdtsc_interfering
   read_big_struct
   remove_latest_trace
   restart_abnormal_exit

src/Monkeypatcher.cc

@@ -1031,17 +1031,28 @@ const syscall_patch_hook* Monkeypatcher::find_syscall_hook(RecordTask* t,
       if (b == 0xeb || (b >= 0x70 && b < 0x80)) {
         int offset_from_instruction_end = (int)i + 2 + (int8_t)bytes[i + 1] -
             (LOOK_BACK + instruction_length);
-        if ((hook.flags & PATCH_IS_MULTIPLE_INSTRUCTIONS)
-                ? (offset_from_instruction_end >= 0 && offset_from_instruction_end < hook.patch_region_length)
-                : offset_from_instruction_end == 0) {
-          LOG(debug) << "Found potential interfering branch at "
-                      << ip.to_data_ptr<uint8_t>() - LOOK_BACK + i;
-          // We can't patch this because it would jump straight back into
-          // the middle of our patch code.
-          found_potential_interfering_branch = true;
-          break;
+        if (hook.flags & PATCH_SYSCALL_INSTRUCTION_IS_LAST) {
+          if (!(hook.flags & PATCH_IS_MULTIPLE_INSTRUCTIONS)) {
+            found_potential_interfering_branch = offset_from_instruction_end == -(ssize_t)instruction_length;
+          } else {
+            found_potential_interfering_branch =
+              offset_from_instruction_end <= -(ssize_t)instruction_length &&
+              offset_from_instruction_end > -(ssize_t)(instruction_length + hook.patch_region_length);
+          }
+        } else {
+          if (!(hook.flags & PATCH_IS_MULTIPLE_INSTRUCTIONS)) {
+            found_potential_interfering_branch = offset_from_instruction_end == 0;
+          } else {
+            found_potential_interfering_branch =
+              offset_from_instruction_end >= 0 && offset_from_instruction_end < hook.patch_region_length;
+          }
         }
       }
+      if (found_potential_interfering_branch) {
+        LOG(debug) << "Found potential interfering branch at "
+                    << ip.to_data_ptr<uint8_t>() - LOOK_BACK + i;
+        break;
+      }
     }
 
     if (!found_potential_interfering_branch) {

src/test/x86/rdtsc_interfering.c

@@ -0,0 +1,38 @@
+/* -*- Mode: C; tab-width: 8; c-basic-offset: 2; indent-tabs-mode: nil; -*- */
+
+#include "util.h"
+
+uint64_t rdtsc_interfering_sideeffect(uint32_t do_jump) {
+  uint64_t check = 0xdeadbeef;
+  uint64_t challenge = 0xfeedbeef;
+  uint32_t out = 0;
+  uint32_t out_hi = 0;
+#ifdef __x86_64__
+  asm volatile (
+              "xor %%rbx, %%rbx\n\t" // Zero rbx
+              "test %%rcx, %%rcx\n\t"
+              // This branch is useless, but let's make sure it works anyway.
+              // In practice, we mostly want to make sure that spurious interfering
+              // branches don't prevent patching so that the `nopl 0(%ax, %ax, 1); rdtsc`
+              // sequence is guaranteed to patch properly.
+              "jnz 1f\n\t"
+              // What instruction exactly this is doesn't really matter, all we
+              // care about is that it has a syscallbuf patch and that we can check
+              // whether it ran or not.
+              "mov %%rdi,%%rbx\n\t"
+              "1: rdtsc\n\t"
+              : "=a"(out), "=d"(out_hi), "=b"(check)
+              : "c"(do_jump), "D"(challenge)
+              : "cc");
+#endif
+  test_assert(check == (do_jump ? 0 : challenge));
+  return ((uint64_t)out_hi << 32) + out;
+}
+
+int main(void) {
+  uint64_t tsc1 = rdtsc_interfering_sideeffect(0);
+  uint64_t tsc2 = rdtsc_interfering_sideeffect(1);
+  test_assert(tsc1 < tsc2);
+  atomic_puts("EXIT-SUCCESS");
+  return 0;
+}

src/test/x86/rdtsc_interfering.run

@@ -0,0 +1,5 @@
+source `dirname $0`/util.sh
+skip_if_no_syscall_buf
+skip_if_test_32_bit
+skip_if_rr_32_bit
+compare_test EXIT-SUCCESS