Skip to content

[win/asan] Recognize mov QWORD PTR [rip + X], reg #117335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

zmodem
Copy link
Collaborator

@zmodem zmodem commented Nov 22, 2024

This comes up when intercepting clang-built __sanitizer_cov functions.

@llvmbot
Copy link
Member

llvmbot commented Nov 22, 2024

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

@llvm/pr-subscribers-platform-windows

Author: Hans (zmodem)

Changes

This comes up when intercepting clang-built __sanitizer_cov functions.


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

1 Files Affected:

  • (modified) compiler-rt/lib/interception/interception_win.cpp (+4)
diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp
index ac81beee11a39c..8b8ce1abe906f6 100644
--- a/compiler-rt/lib/interception/interception_win.cpp
+++ b/compiler-rt/lib/interception/interception_win.cpp
@@ -816,6 +816,10 @@ static size_t GetInstructionSize(uptr address, size_t* rel_offset = nullptr) {
                       //   mov rax, QWORD PTR [rip + XXXXXXXX]
     case 0x058d48:    // 48 8d 05 XX XX XX XX :
                       //   lea rax, QWORD PTR [rip + XXXXXXXX]
+    case 0x0d8948:    // 48 89 0d XX XX XX XX :
+                      //   mov QWORD PTR [rip + XXXXXXXX], rcx
+    case 0x158948:    // 48 89 15 XX XX XX XX :
+                      //   mov QWORD PTR [rip + XXXXXXXX], rdx
     case 0x25ff48:    // 48 ff 25 XX XX XX XX :
                       //   rex.W jmp QWORD PTR [rip + XXXXXXXX]
     case 0x158D4C:    // 4c 8d 15 XX XX XX XX : lea r10, [rip + XX]

@mstorsjo mstorsjo requested a review from alvinhochun November 22, 2024 14:57
@mstorsjo
Copy link
Member

CC @bernhardu who has been poking these things lately.

@aganea
Copy link
Member

aganea commented Nov 22, 2024

I am getting these occassionally as well. Live++ has something similar for decoding prologue instructions, but I think it relies on a more sophisticated disassembler. IDK if we can do something better here than keep adding patterns, and if the ROI is there for that. @MolecularMatters

@tivolo
Copy link

tivolo commented Nov 22, 2024

Since I also need to modify the actual instructions on some platforms, I use Intel's XED.

@bernhardu
Copy link
Contributor

... if we can do something better here than keep adding patterns ...

Hello, I tried to create a prototype to integrate minimal version of bddisasm.
It does not build system changes, just ugly including.
The tests do succeed (x86_64,i686) and some ASan enabled executables also start up (x86_64)
with some instructions forcing to behave like the old version. (Just tested in Wine yet.)
I fear there is plenty of regression potential when more instructions can be handled,
e.g. like those used in some tests and relying to fail in GetInstructionSize.

Could this be the direction to go?
Would their "Apache 2.0" license be compatible to LLVMs "Apache 2.0 with Exception"?

@zmodem
Copy link
Collaborator Author

zmodem commented Nov 25, 2024

Yes, it's somewhat ironic that we're playing whack-a-mole with these instruction encoding, while at the same time LLVM has them all in its .td files and also includes a working disassembler.

So it definitely seems like we could do something more rigorous here. On the other hand, the code was originally intended to just handle a few common function prologues. Maybe we don't want to get too fancy.

Conversely, @rnk has often suggested that we should perhaps move away from win/asan doing this binary patching at all, and instead only do interception at the DLL boundaries. For statically linked symbols, it seems better to link in the symbols we want from the start.

For now, I'll go ahead and merge this one since it's just a small addition to what we currently have.

@zmodem zmodem merged commit 55f5d68 into llvm:main Nov 25, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants