Skip to content

Commit 1f3aa41

Browse files
guy-davidguy-david
committed
[AddressSanitizer] Remove memory effects from functions (llvm#130495)
If left as-is, subsequent optimizations might utilize the possible memory effects and optimize-out the instrumentation. Think of the following case: ``` store i8 4, ptr %shadow call void @llvm.lifetime.start.p0(i64 4, ptr %local) %28 = call void @foo(ptr %local) store i8 -8, ptr %shadow call void @llvm.lifetime.end.p0(i64 4, ptr %local) ``` where `foo` is an external function with `memory(argmem: write)`. A pass such as DeadStoreElimination is allowed to remove the initial store, which might fail sanitizer checks within `foo`. My first attempt was to add a `memory(readwrite)` at the call-site level, but unfortunately the current implementation of `getMemoryEffects` doesn't exactly give it "precedence" as specified, but rather restricts the access specified by the call-site and not the other way around as well.
1 parent d416294 commit 1f3aa41

File tree

5 files changed

+74
-45
lines changed

5 files changed

+74
-45
lines changed

‎llvm/include/llvm/Transforms/Instrumentation/AddressSanitizerCommon.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
5858
bool IsKasan, uint64_t *ShadowBase,
5959
int *MappingScale, bool *OrShadowOffset);
6060

61+
/// Remove memory attributes that are incompatible with the instrumentation
62+
/// added by AddressSanitizer and HWAddressSanitizer.
63+
/// \p ReadsArgMem - indicates whether function arguments may be read by
64+
/// instrumentation and require removing `writeonly` attributes.
65+
void removeASanIncompatibleFnAttributes(Function &F, bool ReadsArgMem);
66+
6167
} // namespace llvm
6268

6369
#endif

‎llvm/lib/Transforms/Instrumentation/AddressSanitizer.cpp

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,45 @@ void getAddressSanitizerParams(const Triple &TargetTriple, int LongSize,
612612
*OrShadowOffset = Mapping.OrShadowOffset;
613613
}
614614

615+
void removeASanIncompatibleFnAttributes(Function &F, bool ReadsArgMem) {
616+
// Sanitizer checks read from shadow, which invalidates memory(argmem: *).
617+
//
618+
// This is not only true for sanitized functions, because AttrInfer can
619+
// infer those attributes on libc functions, which is not true if those
620+
// are instrumented (Android) or intercepted.
621+
//
622+
// We might want to model ASan shadow memory more opaquely to get rid of
623+
// this problem altogether, by hiding the shadow memory write in an
624+
// intrinsic, essentially like in the AArch64StackTagging pass. But that's
625+
// for another day.
626+
627+
// The API is weird. `onlyReadsMemory` actually means "does not write", and
628+
// `onlyWritesMemory` actually means "does not read". So we reconstruct
629+
// "accesses memory" && "does not read" <=> "writes".
630+
bool Changed = false;
631+
if (!F.doesNotAccessMemory()) {
632+
bool WritesMemory = !F.onlyReadsMemory();
633+
bool ReadsMemory = !F.onlyWritesMemory();
634+
if ((WritesMemory && !ReadsMemory) || F.onlyAccessesArgMemory()) {
635+
F.removeFnAttr(Attribute::Memory);
636+
Changed = true;
637+
}
638+
}
639+
if (ReadsArgMem) {
640+
for (Argument &A : F.args()) {
641+
if (A.hasAttribute(Attribute::WriteOnly)) {
642+
A.removeAttr(Attribute::WriteOnly);
643+
Changed = true;
644+
}
645+
}
646+
}
647+
if (Changed) {
648+
// nobuiltin makes sure later passes don't restore assumptions about
649+
// the function.
650+
F.addFnAttr(Attribute::NoBuiltin);
651+
}
652+
}
653+
615654
ASanAccessInfo::ASanAccessInfo(int32_t Packed)
616655
: Packed(Packed),
617656
AccessSizeIndex((Packed >> kAccessSizeIndexShift) & kAccessSizeIndexMask),
@@ -2737,6 +2776,9 @@ int ModuleAddressSanitizer::GetAsanVersion(const Module &M) const {
27372776
bool ModuleAddressSanitizer::instrumentModule(Module &M) {
27382777
initializeCallbacks(M);
27392778

2779+
for (Function &F : M)
2780+
removeASanIncompatibleFnAttributes(F, /*ReadsArgMem=*/false);
2781+
27402782
// Create a module constructor. A destructor is created lazily because not all
27412783
// platforms, and not all modules need it.
27422784
if (ConstructorKind == AsanCtorKind::Global) {

‎llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

Lines changed: 3 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -316,7 +316,6 @@ class HWAddressSanitizer {
316316
FunctionAnalysisManager &FAM) const;
317317
void initializeModule();
318318
void createHwasanCtorComdat();
319-
void removeFnAttributes(Function *F);
320319

321320
void initializeCallbacks(Module &M);
322321

@@ -592,55 +591,17 @@ void HWAddressSanitizer::createHwasanCtorComdat() {
592591
appendToCompilerUsed(M, Dummy);
593592
}
594593

595-
void HWAddressSanitizer::removeFnAttributes(Function *F) {
596-
// Remove memory attributes that are invalid with HWASan.
597-
// HWASan checks read from shadow, which invalidates memory(argmem: *)
598-
// Short granule checks on function arguments read from the argument memory
599-
// (last byte of the granule), which invalidates writeonly.
600-
//
601-
// This is not only true for sanitized functions, because AttrInfer can
602-
// infer those attributes on libc functions, which is not true if those
603-
// are instrumented (Android) or intercepted.
604-
//
605-
// We might want to model HWASan shadow memory more opaquely to get rid of
606-
// this problem altogether, by hiding the shadow memory write in an
607-
// intrinsic, essentially like in the AArch64StackTagging pass. But that's
608-
// for another day.
609-
610-
// The API is weird. `onlyReadsMemory` actually means "does not write", and
611-
// `onlyWritesMemory` actually means "does not read". So we reconstruct
612-
// "accesses memory" && "does not read" <=> "writes".
613-
bool Changed = false;
614-
if (!F->doesNotAccessMemory()) {
615-
bool WritesMemory = !F->onlyReadsMemory();
616-
bool ReadsMemory = !F->onlyWritesMemory();
617-
if ((WritesMemory && !ReadsMemory) || F->onlyAccessesArgMemory()) {
618-
F->removeFnAttr(Attribute::Memory);
619-
Changed = true;
620-
}
621-
}
622-
for (Argument &A : F->args()) {
623-
if (A.hasAttribute(Attribute::WriteOnly)) {
624-
A.removeAttr(Attribute::WriteOnly);
625-
Changed = true;
626-
}
627-
}
628-
if (Changed) {
629-
// nobuiltin makes sure later passes don't restore assumptions about
630-
// the function.
631-
F->addFnAttr(Attribute::NoBuiltin);
632-
}
633-
}
634-
635594
/// Module-level initialization.
636595
///
637596
/// inserts a call to __hwasan_init to the module's constructor list.
638597
void HWAddressSanitizer::initializeModule() {
639598
LLVM_DEBUG(dbgs() << "Init " << M.getName() << "\n");
640599
TargetTriple = Triple(M.getTargetTriple());
641600

601+
// HWASan may do short granule checks on function arguments read from the
602+
// argument memory (last byte of the granule), which invalidates writeonly.
642603
for (Function &F : M.functions())
643-
removeFnAttributes(&F);
604+
removeASanIncompatibleFnAttributes(F, /*ReadsArgMem=*/true);
644605

645606
// x86_64 currently has two modes:
646607
// - Intel LAM (default)

‎llvm/test/Instrumentation/AddressSanitizer/AMDGPU/asan_instrument_mem_intrinsics.ll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ declare void @llvm.memset.p5.i32(ptr addrspace(5) nocapture writeonly, i8, i32,
186186

187187
define void @test_mem_intrinsic_memcpy(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
188188
entry:
189-
;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
189+
;CHECK: define void @test_mem_intrinsic_memcpy(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
190190
;CHECK-NEXT: entry:
191191
;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memcpy(ptr [[DEST0]], ptr [[SRC0]], i64 64)
192192
;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -469,7 +469,7 @@ entry:
469469

470470
define void @test_mem_intrinsic_memmove(ptr %dest0,ptr %src0,ptr addrspace(1) %dest1,ptr addrspace(1) %src1,ptr addrspace(2) %dest2,ptr addrspace(2) %src2,ptr addrspace(3) %dest3,ptr addrspace(3) %src3,ptr addrspace(4) %dest4,ptr addrspace(4) %src4,ptr addrspace(5) %dest5,ptr addrspace(5) %src5) #0 {
471471
entry:
472-
;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]) #2 {
472+
;CHECK: define void @test_mem_intrinsic_memmove(ptr [[DEST0:%.*]], ptr [[SRC0:%.*]], ptr addrspace(1) [[DEST1:%.*]], ptr addrspace(1) [[SRC1:%.*]], ptr addrspace(2) [[DEST2:%.*]], ptr addrspace(2) [[SRC2:%.*]], ptr addrspace(3) [[DEST3:%.*]], ptr addrspace(3) [[SRC3:%.*]], ptr addrspace(4) [[DEST4:%.*]], ptr addrspace(4) [[SRC4:%.*]], ptr addrspace(5) [[DEST5:%.*]], ptr addrspace(5) [[SRC5:%.*]]){{.*}} {
473473
;CHECK-NEXT: entry:
474474
;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memmove(ptr [[DEST0]], ptr [[SRC0]], i64 64)
475475
;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[SRC1]] to ptr
@@ -753,7 +753,7 @@ entry:
753753

754754
define void @test_mem_intrinsic_memset(ptr %ptr0,ptr addrspace(1) %ptr1,ptr addrspace(2) %ptr2,ptr addrspace(3) %ptr3,ptr addrspace(4) %ptr4,ptr addrspace(5) %ptr5) #0{
755755
entry:
756-
;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]) #2 {
756+
;CHECK: define void @test_mem_intrinsic_memset(ptr [[PTR0:%.*]], ptr addrspace(1) [[PTR1:%.*]], ptr addrspace(2) [[PTR2:%.*]], ptr addrspace(3) [[PTR3:%.*]], ptr addrspace(4) [[PTR4:%.*]], ptr addrspace(5) [[PTR5:%.*]]){{.*}} {
757757
;CHECK-NEXT: entry:
758758
;CHECK-NEXT: [[VR0:%.*]] = call ptr @__asan_memset(ptr [[PTR0]], i32 1, i64 128)
759759
;CHECK-NEXT: [[VR1:%.*]] = addrspacecast ptr addrspace(1) [[PTR1]] to ptr

‎llvm/test/Instrumentation/AddressSanitizer/remove-memory-effects.ll

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
; Remove possible memory effects from functions that are invalidated by
2+
; AddressSanitizer instrumentation.
3+
4+
; RUN: opt -passes='asan<use-after-scope>' -S %s | FileCheck %s
5+
6+
; CHECK: @foo(ptr writeonly) #[[ATTRS_FOO:[0-9]+]]
7+
declare void @foo(ptr writeonly) memory(argmem: write)
8+
9+
; CHECK: @bar() #[[ATTRS_BAR:[0-9]+]]
10+
define void @bar() sanitize_address {
11+
entry:
12+
%x = alloca i32, align 4
13+
call void @llvm.lifetime.start.p0(i64 4, ptr %x)
14+
call void @foo(ptr %x)
15+
call void @llvm.lifetime.end.p0(i64 4, ptr %x)
16+
ret void
17+
}
18+
19+
; CHECK: attributes #[[ATTRS_FOO]] = { nobuiltin }
20+
; CHECK: attributes #[[ATTRS_BAR]] = { sanitize_address }

0 commit comments

Comments
 (0)