[ASan][Windows][x86] Modify IsMemoryPadding to Check for Possible Shortjump Corruption

[39otsu]

The current IsMemoryPadding implementation assumes that any five 0xCC bytes is suitable for patching. This leaves open the possibility of corruption if the first 0xCC is part of a short jump when attempting to override function with the detour technique.
Example:

EB CC // short jump with cc offset CC CC CC CC // padding 8B FF // mov edi, edi start of another function 

While a more considerate implementation is to scan through an arbitrary number of previous addresses to ensure the padding is suitable, this fix will simply check the preceding byte in case of that possible short jump scenario.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

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

Author: None (39otsu)

Changes

The current IsMemoryPadding implementation assumes that any five 0xCC bytes is suitable for patching. This leaves open the possibility of corruption if the first 0xCC is part of a short jump when attempting to override function with the detour technique.

While a more considerate implementation is to scan through an arbitrary number of previous addresses to ensure the padding is suitable, this fix will simply check the preceding byte in case of that possible short jump scenario.

---

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

3 Files Affected:

• (modified) compiler-rt/lib/interception/interception_win.cpp (+8)
• (added) compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.asm (+35)
• (added) compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp (+60)

diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp index 246a22c56c31a..a273501a75a3f 100644 --- a/compiler-rt/lib/interception/interception_win.cpp +++ b/compiler-rt/lib/interception/interception_win.cpp @@ -254,6 +254,14 @@ static bool RestoreMemoryProtection( static bool IsMemoryPadding(uptr address, uptr size) { u8* function = (u8*)address; + if (function[-1] == 0xEB) { + // Check for short jump instruction. + // There is a rare instance of a short jump (i.e. 0xEB) containing a 0xCC offset placed + // right before 4 bytes of 0xCC padding. This conditional will prevent an  + // erroneous detour function override by falling through to use  + // another interception technique. + return false; + } for (size_t i = 0; i < size; ++i) if (function[i] != 0x90 && function[i] != 0xCC) return false; diff --git a/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.asm b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.asm new file mode 100644 index 0000000000000..1c5c66f195fce --- /dev/null +++ b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.asm @@ -0,0 +1,35 @@ +.386  + +_text segment para 'CODE' ; Start the code segment.  + align 16 + public _false_header + public _function_to_intercept + +_false_header proc +cc: + db 66h, 90h ; Padding function to force offset to 0xCC + nop dword ptr [eax+eax*1+10000000h]  + nop dword ptr [eax+eax*1+10000000h] + nop dword ptr [eax+eax*1+10000000h] + nop dword ptr [eax+eax*1+10000000h] + nop dword ptr [eax+eax*1+10000000h] + nop dword ptr [eax+eax*1+10000000h] + jmp cc ; Short jump with a 0xCC byte used as an offset + int 3 ; 4 bytes of 0xCC padding + int 3 + int 3 + int 3 +_false_header endp + +_function_to_intercept proc + mov edi, edi ; Function to be overridden + push ebp + mov ebp, esp + mov eax, 0  + pop ebp + ret  +_function_to_intercept endp + +_text ends  + +end \ No newline at end of file diff --git a/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp new file mode 100644 index 0000000000000..529e3412f7ee3 --- /dev/null +++ b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp @@ -0,0 +1,60 @@ +// RUN: ml /c /coff /Fo%t_asm.obj %p/intercept_detour_minus_one.asm +// RUN: %clang_cl -Od %s %t_asm.obj -Fe%t /link /INFERASANLIBS +// RUN: %run %t 2>&1 | FileCheck %s + +// This test is for the Windows 32bit-specific interception technique detour.  +// There is a rare instance of a short jump containing a 0xCC offset placed +// right before 4 bytes of 0xCC padding. This test checks that the +// detour function override is not applied in that instance. +// UNSUPPORTED: asan-64-bits + +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <stdint.h> +#include <algorithm> + +extern "C" __declspec(dllimport) +bool __cdecl __sanitizer_override_function_by_addr( + void *source_function, + void *target_function, + void **old_target_function = nullptr + ); + +template <typename F> +F *apply_interception(const F& source, const F& target) { + void *old_default = nullptr; + if (!__sanitizer_override_function_by_addr(&source, &target, &old_default)) { + fputs("__sanitizer_override_function_by_addr failed.", stderr); // CHECK-NOT: __sanitizer_override_function_by_addr failed. + exit(1); + } + return reinterpret_cast<F*>(old_default); +} + +extern "C" bool validate_interception(void * addr) { + // Checks if 5 preceding bytes have been + // corrupted by the interception. + auto p = static_cast<const uint8_t*>(addr); // use uint8_t for byte-wise access + return std::all_of(p - 5, p, [](uint8_t byte) { + // 0xCC: INT3 (breakpoint instruction), used for padding or debugging + // 0x90: NOP (no operation), used for padding or alignment + return byte == 0xCC || byte == 0x90; + }); +} + +int dummy_function() { + // Dummy function to overriding with. + // It should not be called directly in this test. + return 0; +} +extern "C" int false_header(); +extern "C" int function_to_intercept(); + +int main() { + auto func = apply_interception(dummy_function, function_to_intercept); + if (validate_interception((void*)function_to_intercept)) { + printf("Success\n"); // CHECK: Success + return 0; + } + return 1; +} \ No newline at end of file 

[efriedma-quic]

Why are short jumps special, as opposed to some other instruction ending in 0x90/0xCC, like a call?

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions cpp -- compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp compiler-rt/lib/interception/interception_win.cpp

View the diff from clang-format here.

diff --git a/compiler-rt/lib/interception/interception_win.cpp b/compiler-rt/lib/interception/interception_win.cpp index a273501a7..3710e786c 100644 --- a/compiler-rt/lib/interception/interception_win.cpp +++ b/compiler-rt/lib/interception/interception_win.cpp @@ -254,11 +254,11 @@ static bool RestoreMemoryProtection( static bool IsMemoryPadding(uptr address, uptr size) { u8* function = (u8*)address; - if (function[-1] == 0xEB) { + if (function[-1] == 0xEB) { // Check for short jump instruction. - // There is a rare instance of a short jump (i.e. 0xEB) containing a 0xCC offset placed - // right before 4 bytes of 0xCC padding. This conditional will prevent an  - // erroneous detour function override by falling through to use  + // There is a rare instance of a short jump (i.e. 0xEB) containing a 0xCC + // offset placed right before 4 bytes of 0xCC padding. This conditional will + // prevent an erroneous detour function override by falling through to use // another interception technique. return false; } diff --git a/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp index 529e3412f..41008f4c2 100644 --- a/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp +++ b/compiler-rt/test/asan/TestCases/Windows/intercept_detour_minus_one.cpp @@ -2,59 +2,58 @@ // RUN: %clang_cl -Od %s %t_asm.obj -Fe%t /link /INFERASANLIBS // RUN: %run %t 2>&1 | FileCheck %s -// This test is for the Windows 32bit-specific interception technique detour.  +// This test is for the Windows 32bit-specific interception technique detour. // There is a rare instance of a short jump containing a 0xCC offset placed // right before 4 bytes of 0xCC padding. This test checks that the // detour function override is not applied in that instance. // UNSUPPORTED: asan-64-bits +#include <algorithm> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <string.h> -#include <stdint.h> -#include <algorithm> -extern "C" __declspec(dllimport) -bool __cdecl __sanitizer_override_function_by_addr( - void *source_function, - void *target_function, - void **old_target_function = nullptr - ); +extern "C" + __declspec(dllimport) bool __cdecl __sanitizer_override_function_by_addr( + void *source_function, void *target_function, + void **old_target_function = nullptr); -template <typename F> -F *apply_interception(const F& source, const F& target) { - void *old_default = nullptr; - if (!__sanitizer_override_function_by_addr(&source, &target, &old_default)) { - fputs("__sanitizer_override_function_by_addr failed.", stderr); // CHECK-NOT: __sanitizer_override_function_by_addr failed. - exit(1); - } - return reinterpret_cast<F*>(old_default); +template <typename F> F *apply_interception(const F &source, const F &target) { + void *old_default = nullptr; + if (!__sanitizer_override_function_by_addr(&source, &target, &old_default)) { + fputs("__sanitizer_override_function_by_addr failed.", + stderr); // CHECK-NOT: __sanitizer_override_function_by_addr failed. + exit(1); + } + return reinterpret_cast<F *>(old_default); } -extern "C" bool validate_interception(void * addr) { - // Checks if 5 preceding bytes have been - // corrupted by the interception. - auto p = static_cast<const uint8_t*>(addr); // use uint8_t for byte-wise access - return std::all_of(p - 5, p, [](uint8_t byte) { - // 0xCC: INT3 (breakpoint instruction), used for padding or debugging - // 0x90: NOP (no operation), used for padding or alignment - return byte == 0xCC || byte == 0x90; - }); +extern "C" bool validate_interception(void *addr) { + // Checks if 5 preceding bytes have been + // corrupted by the interception. + auto p = + static_cast<const uint8_t *>(addr); // use uint8_t for byte-wise access + return std::all_of(p - 5, p, [](uint8_t byte) { + // 0xCC: INT3 (breakpoint instruction), used for padding or debugging + // 0x90: NOP (no operation), used for padding or alignment + return byte == 0xCC || byte == 0x90; + }); } int dummy_function() { - // Dummy function to overriding with. - // It should not be called directly in this test. - return 0; + // Dummy function to overriding with. + // It should not be called directly in this test. + return 0; } extern "C" int false_header(); extern "C" int function_to_intercept(); int main() { - auto func = apply_interception(dummy_function, function_to_intercept); - if (validate_interception((void*)function_to_intercept)) { - printf("Success\n"); // CHECK: Success - return 0; - } - return 1; + auto func = apply_interception(dummy_function, function_to_intercept); + if (validate_interception((void *)function_to_intercept)) { + printf("Success\n"); // CHECK: Success + return 0; + } + return 1; } \ No newline at end of file