Skip to content

[Sanitizers][Darwin] Pass offset to __asan_set_shadow_xx #71745

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 12, 2023

Conversation

wrotki
Copy link
Contributor

@wrotki wrotki commented Nov 8, 2023

Normally, when __asan_option_detect_stack_use_after_return option is set,
the instrumentation passed the adress of the shadow memory bytes to be set, for detecting problems with local variables.
This can be a problem when the -fsanitize-stable-abi option is in effect,
since the ABI implementation doesn't have means to communicate the current shadow memory base address
back to its users.

This change addresses it simply by setting __asan_shadow_memory_dynamic_address to zero. It means
that __asan_set_shadow_xx will be now called with the offset relative to the current shadow memory
base, and the ABI implementation needs to adapt accordingly.

The other change here is to set __asan_option_detect_stack_use_after_return to nonzer by default,
which is needed for instrumentation to take paths using the __asan_shadow_memory_dynamic_address
and __asan_set_shadow_xx calls.

Normally, when __asan_option_detect_stack_use_after_return option is set,
the instrumentation passed the adress of the shadow memory bytes to be set, for detecting problems with local variables.
This can be a problem when the -fsanitize-stable-abi option is in effect,
since the ABI implementation doesn't have means to communicate the current shadow memory base address
back to its users.

This change addresses it simply by setting __asan_shadow_memory_dynamic_address to zero. It means
that __asan_set_shadow_xx will be now called with the offset relative to the current shadow memory
base, and need to adapt accordingly.

The other change here is to set __asan_option_detect_stack_use_after_return to nonzer by default,
which is needed for instrumentation to take paths using the __asan_shadow_memory_dynamic_address
and __asan_set_shadow_xx calls.
@llvmbot
Copy link
Member

llvmbot commented Nov 8, 2023

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

Author: Mariusz Borsa (wrotki)

Changes

Normally, when __asan_option_detect_stack_use_after_return option is set,
the instrumentation passed the adress of the shadow memory bytes to be set, for detecting problems with local variables.
This can be a problem when the -fsanitize-stable-abi option is in effect,
since the ABI implementation doesn't have means to communicate the current shadow memory base address
back to its users.

This change addresses it simply by setting __asan_shadow_memory_dynamic_address to zero. It means
that __asan_set_shadow_xx will be now called with the offset relative to the current shadow memory
base, and need to adapt accordingly.

The other change here is to set __asan_option_detect_stack_use_after_return to nonzer by default,
which is needed for instrumentation to take paths using the __asan_shadow_memory_dynamic_address
and __asan_set_shadow_xx calls.


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

1 Files Affected:

  • (modified) compiler-rt/lib/asan_abi/asan_abi_shim.cpp (+2-2)
diff --git a/compiler-rt/lib/asan_abi/asan_abi_shim.cpp b/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
index bf3cba3178efbdf..35c45dff96f6d2f 100644
--- a/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
+++ b/compiler-rt/lib/asan_abi/asan_abi_shim.cpp
@@ -65,8 +65,8 @@ void __asan_handle_no_return(void) { __asan_abi_handle_no_return(); }
 
 // Variables concerning RTL state. These provisionally exist for completeness
 // but will likely move into the Stable ABI implementation and not in the shim.
-uptr __asan_shadow_memory_dynamic_address = (uptr)0xdeaddeaddeadbeaf;
-int __asan_option_detect_stack_use_after_return = 0;
+uptr __asan_shadow_memory_dynamic_address = (uptr)0L;
+int __asan_option_detect_stack_use_after_return = 1;
 
 // Functions concerning memory load and store reporting
 void __asan_report_load1(uptr addr) {

@wrotki wrotki requested review from rsundahl, yln and usama54321 November 8, 2023 23:27
Copy link
Contributor

@rsundahl rsundahl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. It's the simplest way to get what you want without changing the instrumentation pass to use a constant instead of a variable when -fsanitize-stable-abi is true. We might want to go there in the future though for the performance advantages.

@rsundahl
Copy link
Contributor

rsundahl commented Nov 9, 2023

cc: @kcc and @vitalybuka... (I don't think that it's ideal that the instrumentation is really even aware of the shadow memory when -fsanitize-stable-abi in the sense that the implementation of the stable ABI would preferably be implementation agnostic but that's a bigger lift than I think we can make atm.)

@wrotki wrotki merged commit c2205ab into llvm:main Nov 12, 2023
@wrotki wrotki deleted the pass_offset_to_shadow_base branch November 12, 2023 22:44
wrotki added a commit to swiftlang/llvm-project that referenced this pull request Nov 13, 2023
Normally, when __asan_option_detect_stack_use_after_return option is
set,
the instrumentation passed the adress of the shadow memory bytes to be
set, for detecting problems with local variables.
This can be a problem when the -fsanitize-stable-abi option is in
effect,
since the ABI implementation doesn't have means to communicate the
current shadow memory base address
back to its users.

This change addresses it simply by setting
__asan_shadow_memory_dynamic_address to zero. It means
that __asan_set_shadow_xx will be now called with the offset relative to
the current shadow memory
base, and the ABI implementation needs to adapt accordingly.

The other change here is to set
__asan_option_detect_stack_use_after_return to nonzer by default,
which is needed for instrumentation to take paths using the
__asan_shadow_memory_dynamic_address
and __asan_set_shadow_xx calls.

Co-authored-by: Mariusz Borsa <[email protected]>
(cherry picked from commit c2205ab)
wrotki added a commit to swiftlang/llvm-project that referenced this pull request Nov 15, 2023
[Cherry-pick into stable/20230725][Sanitizers][Darwin] Pass offset to __asan_set_shadow_xx (llvm#71745)
zahiraam pushed a commit to zahiraam/llvm-project that referenced this pull request Nov 20, 2023
Normally, when __asan_option_detect_stack_use_after_return option is
set,
the instrumentation passed the adress of the shadow memory bytes to be
set, for detecting problems with local variables.
This can be a problem when the -fsanitize-stable-abi option is in
effect,
since the ABI implementation doesn't have means to communicate the
current shadow memory base address
back to its users.

This change addresses it simply by setting
__asan_shadow_memory_dynamic_address to zero. It means
that __asan_set_shadow_xx will be now called with the offset relative to
the current shadow memory
base, and the ABI implementation needs to adapt accordingly.

The other change here is to set
__asan_option_detect_stack_use_after_return to nonzer by default,
which is needed for instrumentation to take paths using the
__asan_shadow_memory_dynamic_address
and __asan_set_shadow_xx calls.

Co-authored-by: Mariusz Borsa <[email protected]>
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Nov 23, 2023
Local branch amd-gfx cfb0aab Merged main:4c3206c5d5de into amd-gfx:66b766bb118d
Remote branch main c2205ab [Sanitizers][Darwin] Pass offset to __asan_set_shadow_xx (llvm#71745)
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.

3 participants