Skip to content

[msan] Unpoison indirect outputs for userspace when -msan-handle-asm-conservative is specified #77393

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.

Sign up for GitHub

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 2 commits into from
Jan 20, 2024
Merged

[msan] Unpoison indirect outputs for userspace when -msan-handle-asm-conservative is specified #77393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93ab631
[𝘀𝗽𝗿] initial version
MaskRay Jan 8, 2024
53fc59b
Add a comment about the alignment
MaskRay Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4103,7 +4103,11 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
// do the usual thing: check argument shadow and mark all outputs as
// clean. Note that any side effects of the inline asm that are not
// immediately visible in its constraints are not handled.
if (ClHandleAsmConservative && MS.CompileKernel)
// For now, handle inline asm by default for KMSAN.
bool HandleAsm = ClHandleAsmConservative.getNumOccurrences()
? ClHandleAsmConservative
: MS.CompileKernel;
if (HandleAsm)
visitAsmInstruction(CB);
else
visitInstruction(CB);
Expand Down Expand Up @@ -4557,7 +4561,15 @@ struct MemorySanitizerVisitor : public InstVisitor<MemorySanitizerVisitor> {
return;
Value *SizeVal =
IRB.CreateTypeSize(MS.IntptrTy, DL.getTypeStoreSize(ElemTy));
IRB.CreateCall(MS.MsanInstrumentAsmStoreFn, {Operand, SizeVal});
if (MS.CompileKernel) {
IRB.CreateCall(MS.MsanInstrumentAsmStoreFn, {Operand, SizeVal});
} else {
// ElemTy, derived from elementtype(), does not encode the alignment of
// the pointer. Conservatively assume that the shadow memory is unaligned.
auto [ShadowPtr, _] =
getShadowOriginPtrUserspace(Operand, IRB, IRB.getInt8Ty(), Align(1));
IRB.CreateAlignedStore(getCleanShadow(ElemTy), ShadowPtr, Align(1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we use the the Aligned variant? Align(1) should be the same as unspecified alignment, so we can just use CreateStore, right?

Copy link
Member Author

@MaskRay MaskRay Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

ElemTy utilizes elementtype (ptr elementtype(i32) @id1 => i32, which does not encode the alignment).
An unspecified alignment uses the default, ElemTy's alignment, which is 4 in the i32 case.

However, if the element type is actually unaligned, CreateStore-generated align 4 shadow access could cause a misalignment failure on certain architectures using -mstrict-align.

The tests specifically check , align 1

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add this as a comment? This is quite subtle and very likely lost on cursory readers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done:)

}
}

/// Get the number of output arguments returned by pointers.
Expand Down
106 changes: 59 additions & 47 deletions llvm/test/Instrumentation/MemorySanitizer/msan_asm_conservative.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
; Test for handling of asm constraints in MSan instrumentation.
; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | \
; RUN: FileCheck %s
; RUN: opt < %s -msan-check-access-address=0 -msan-handle-asm-conservative=1 -S -passes=msan 2>&1 | \
; RUN: FileCheck --check-prefixes=CHECK,USER-CONS %s
; RUN: opt < %s -msan-kernel=1 -msan-check-access-address=0 \
; RUN: -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | FileCheck \
; RUN: "-check-prefix=CHECK" %s
; RUN: -msan-handle-asm-conservative=0 -S -passes=msan 2>&1 | FileCheck \
; RUN: --check-prefixes=CHECK,KMSAN %s
; RUN: opt < %s -msan-kernel=1 -msan-check-access-address=0 \
; RUN: -msan-handle-asm-conservative=1 -S -passes=msan 2>&1 | FileCheck \
; RUN: "-check-prefixes=CHECK,CHECK-CONS" %s
; RUN: -msan-handle-asm-conservative=1 -S -passes=msan 2>&1 | FileCheck \
; RUN: --check-prefixes=CHECK,KMSAN,CHECK-CONS %s

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
Expand Down Expand Up @@ -46,9 +50,9 @@ entry:
; CHECK: [[IS1_F1:%.*]] = load i32, ptr @is1, align 4
; CHECK: call void @__msan_warning
; CHECK: call i32 asm "",{{.*}}(i32 [[IS1_F1]])
; CHECK: [[PACK1_F1:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; CHECK: [[EXT1_F1:%.*]] = extractvalue { ptr, ptr } [[PACK1_F1]], 0
; CHECK: store i32 0, ptr [[EXT1_F1]]
; KMSAN: [[PACK1_F1:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; KMSAN: [[EXT1_F1:%.*]] = extractvalue { ptr, ptr } [[PACK1_F1]], 0
; KMSAN: store i32 0, ptr [[EXT1_F1]]


; Two input registers, two output registers:
Expand All @@ -69,14 +73,14 @@ entry:
; CHECK: [[IS1_F2:%.*]] = load i32, ptr @is1, align 4
; CHECK: [[IS2_F2:%.*]] = load i32, ptr @is2, align 4
; CHECK: call void @__msan_warning
; CHECK: call void @__msan_warning
; KMSAN: call void @__msan_warning
; CHECK: call { i32, i32 } asm "",{{.*}}(i32 [[IS1_F2]], i32 [[IS2_F2]])
; CHECK: [[PACK1_F2:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; CHECK: [[EXT1_F2:%.*]] = extractvalue { ptr, ptr } [[PACK1_F2]], 0
; CHECK: store i32 0, ptr [[EXT1_F2]]
; CHECK: [[PACK2_F2:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; CHECK: [[EXT2_F2:%.*]] = extractvalue { ptr, ptr } [[PACK2_F2]], 0
; CHECK: store i32 0, ptr [[EXT2_F2]]
; KMSAN: [[PACK1_F2:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; KMSAN: [[EXT1_F2:%.*]] = extractvalue { ptr, ptr } [[PACK1_F2]], 0
; KMSAN: store i32 0, ptr [[EXT1_F2]]
; KMSAN: [[PACK2_F2:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; KMSAN: [[EXT2_F2:%.*]] = extractvalue { ptr, ptr } [[PACK2_F2]], 0
; KMSAN: store i32 0, ptr [[EXT2_F2]]

; Input same as output, used twice:
; asm("" : "=r" (id1), "=r" (id2) : "r" (id1), "r" (id2));
Expand All @@ -96,14 +100,14 @@ entry:
; CHECK: [[ID1_F3:%.*]] = load i32, ptr @id1, align 4
; CHECK: [[ID2_F3:%.*]] = load i32, ptr @id2, align 4
; CHECK: call void @__msan_warning
; CHECK: call void @__msan_warning
; KMSAN: call void @__msan_warning
; CHECK: call { i32, i32 } asm "",{{.*}}(i32 [[ID1_F3]], i32 [[ID2_F3]])
; CHECK: [[PACK1_F3:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; CHECK: [[EXT1_F3:%.*]] = extractvalue { ptr, ptr } [[PACK1_F3]], 0
; CHECK: store i32 0, ptr [[EXT1_F3]]
; CHECK: [[PACK2_F3:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; CHECK: [[EXT2_F3:%.*]] = extractvalue { ptr, ptr } [[PACK2_F3]], 0
; CHECK: store i32 0, ptr [[EXT2_F3]]
; KMSAN: [[PACK1_F3:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; KMSAN: [[EXT1_F3:%.*]] = extractvalue { ptr, ptr } [[PACK1_F3]], 0
; KMSAN: store i32 0, ptr [[EXT1_F3]]
; KMSAN: [[PACK2_F3:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; KMSAN: [[EXT2_F3:%.*]] = extractvalue { ptr, ptr } [[PACK2_F3]], 0
; KMSAN: store i32 0, ptr [[EXT2_F3]]


; One of the input registers is also an output:
Expand All @@ -124,14 +128,14 @@ entry:
; CHECK: [[ID1_F4:%.*]] = load i32, ptr @id1, align 4
; CHECK: [[IS1_F4:%.*]] = load i32, ptr @is1, align 4
; CHECK: call void @__msan_warning
; CHECK: call void @__msan_warning
; KMSAN: call void @__msan_warning
; CHECK: call { i32, i32 } asm "",{{.*}}(i32 [[ID1_F4]], i32 [[IS1_F4]])
; CHECK: [[PACK1_F4:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; CHECK: [[EXT1_F4:%.*]] = extractvalue { ptr, ptr } [[PACK1_F4]], 0
; CHECK: store i32 0, ptr [[EXT1_F4]]
; CHECK: [[PACK2_F4:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; CHECK: [[EXT2_F4:%.*]] = extractvalue { ptr, ptr } [[PACK2_F4]], 0
; CHECK: store i32 0, ptr [[EXT2_F4]]
; KMSAN: [[PACK1_F4:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; KMSAN: [[EXT1_F4:%.*]] = extractvalue { ptr, ptr } [[PACK1_F4]], 0
; KMSAN: store i32 0, ptr [[EXT1_F4]]
; KMSAN: [[PACK2_F4:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; KMSAN: [[EXT2_F4:%.*]] = extractvalue { ptr, ptr } [[PACK2_F4]], 0
; KMSAN: store i32 0, ptr [[EXT2_F4]]


; One input register, three output registers:
Expand All @@ -153,15 +157,15 @@ entry:
; CHECK: [[IS1_F5:%.*]] = load i32, ptr @is1, align 4
; CHECK: call void @__msan_warning
; CHECK: call { i32, i32, i32 } asm "",{{.*}}(i32 [[IS1_F5]])
; CHECK: [[PACK1_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; CHECK: [[EXT1_F5:%.*]] = extractvalue { ptr, ptr } [[PACK1_F5]], 0
; CHECK: store i32 0, ptr [[EXT1_F5]]
; CHECK: [[PACK2_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; CHECK: [[EXT2_F5:%.*]] = extractvalue { ptr, ptr } [[PACK2_F5]], 0
; CHECK: store i32 0, ptr [[EXT2_F5]]
; CHECK: [[PACK3_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id3{{.*}})
; CHECK: [[EXT3_F5:%.*]] = extractvalue { ptr, ptr } [[PACK3_F5]], 0
; CHECK: store i32 0, ptr [[EXT3_F5]]
; KMSAN: [[PACK1_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id1{{.*}})
; KMSAN: [[EXT1_F5:%.*]] = extractvalue { ptr, ptr } [[PACK1_F5]], 0
; KMSAN: store i32 0, ptr [[EXT1_F5]]
; KMSAN: [[PACK2_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id2{{.*}})
; KMSAN: [[EXT2_F5:%.*]] = extractvalue { ptr, ptr } [[PACK2_F5]], 0
; KMSAN: store i32 0, ptr [[EXT2_F5]]
; KMSAN: [[PACK3_F5:%.*]] = call {{.*}} @__msan_metadata_ptr_for_store_4({{.*}}@id3{{.*}})
; KMSAN: [[EXT3_F5:%.*]] = extractvalue { ptr, ptr } [[PACK3_F5]], 0
; KMSAN: store i32 0, ptr [[EXT3_F5]]


; 2 input memory args, 2 output memory args:
Expand All @@ -173,6 +177,8 @@ entry:
}

; CHECK-LABEL: @f_2i_2o_mem
; USER-CONS: store i32 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @id1 to i64), i64 87960930222080) to ptr), align 1
; USER-CONS: store i32 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @id2 to i64), i64 87960930222080) to ptr), align 1
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id1{{.*}}, i64 4)
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id2{{.*}}, i64 4)
; CHECK: call void asm "", "=*m,=*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32) @id1, ptr elementtype(i32) @id2, ptr elementtype(i32) @is1, ptr elementtype(i32) @is2)
Expand All @@ -190,6 +196,7 @@ entry:

; CHECK-LABEL: @f_1i_1o_memreg
; CHECK: [[IS1_F7:%.*]] = load i32, ptr @is1, align 4
; USER-CONS: store i32 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @id1 to i64), i64 87960930222080) to ptr), align 1
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@id1{{.*}}, i64 4)
; CHECK: call void @__msan_warning
; CHECK: call i32 asm "", "=r,=*m,r,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32) @id1, i32 [[IS1_F7]], ptr elementtype(i32) @is1)
Expand All @@ -208,6 +215,7 @@ entry:
}

; CHECK-LABEL: @f_3o_reg_mem_reg
; USER-CONS: store i32 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @id2 to i64), i64 87960930222080) to ptr), align 1
; CHECK-CONS: call void @__msan_instrument_asm_store(ptr @id2, i64 4)
; CHECK: call { i32, i32 } asm "", "=r,=*m,=r,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(i32) @id2)

Expand All @@ -232,10 +240,11 @@ entry:
; CHECK: [[PAIR1_F9:%.*]] = load {{.*}} @pair1
; CHECK: [[C1_F9:%.*]] = load {{.*}} @c1
; CHECK: [[MEMCPY_S1_F9:%.*]] = load {{.*}} @memcpy_s1
; USER-CONS: store { i32, i32 } zeroinitializer, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @pair2 to i64), i64 87960930222080) to ptr), align 1
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@pair2{{.*}}, i64 8)
; CHECK: call void @__msan_warning
; CHECK: call void @__msan_warning
; CHECK: call void @__msan_warning
; KMSAN: call void @__msan_warning
; KMSAN: call void @__msan_warning
; CHECK: call { i8, ptr } asm "", "=*r,=r,=r,r,r,r,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(%struct.pair) @pair2, {{.*}}[[PAIR1_F9]], i8 [[C1_F9]], {{.*}} [[MEMCPY_S1_F9]])

; Three inputs and three outputs of different types: a pair, a char, a function pointer.
Expand All @@ -248,9 +257,12 @@ entry:
}

; CHECK-LABEL: @f_3i_3o_complex_mem
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@pair2{{.*}}, i64 8)
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@c2{{.*}}, i64 1)
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@memcpy_d1{{.*}}, i64 8)
; USER-CONS: store { i32, i32 } zeroinitializer, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @pair2 to i64), i64 87960930222080) to ptr), align 1
; USER-CONS-NEXT: store i8 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @c2 to i64), i64 87960930222080) to ptr), align 1
; USER-CONS-NEXT: store i64 0, ptr inttoptr (i64 xor (i64 ptrtoint (ptr @memcpy_d1 to i64), i64 87960930222080) to ptr), align 1
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@pair2{{.*}}, i64 8)
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@c2{{.*}}, i64 1)
; CHECK-CONS: call void @__msan_instrument_asm_store({{.*}}@memcpy_d1{{.*}}, i64 8)
; CHECK: call void asm "", "=*m,=*m,=*m,*m,*m,*m,~{dirflag},~{fpsr},~{flags}"(ptr elementtype(%struct.pair) @pair2, ptr elementtype(i8) @c2, ptr elementtype(ptr) @memcpy_d1, ptr elementtype(%struct.pair) @pair1, ptr elementtype(i8) @c1, ptr elementtype(ptr) @memcpy_s1)


Expand Down Expand Up @@ -278,8 +290,8 @@ cleanup: ; preds = %entry, %skip_label
}

; CHECK-LABEL: @asm_goto
; CHECK: [[LOAD_ARG:%.*]] = load {{.*}} %_msarg
; CHECK: [[CMP:%.*]] = icmp ne {{.*}} [[LOAD_ARG]], 0
; CHECK: br {{.*}} [[CMP]], label %[[LABEL:.*]], label
; CHECK: [[LABEL]]:
; CHECK-NEXT: call void @__msan_warning
; KMSAN: [[LOAD_ARG:%.*]] = load {{.*}} %_msarg
; KMSAN: [[CMP:%.*]] = icmp ne {{.*}} [[LOAD_ARG]], 0
; KMSAN: br {{.*}} [[CMP]], label %[[LABEL:.*]], label
; KMSAN: [[LABEL]]:
; KMSAN-NEXT: call void @__msan_warning