Skip to content
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

Jump to bottom

MSAN: detect *all* uninitialized reads? #883

Open
LebedevRI opened this issue Nov 19, 2017 · 10 comments
Open

MSAN: detect *all* uninitialized reads? #883

LebedevRI opened this issue Nov 19, 2017 · 10 comments

Comments

@LebedevRI
Copy link

Hi.
This is related to google/oss-fuzz#863

I'm fuzzing a RawSpeed library.
Memory sanitizer is being extremely useful to find one particular set
of issues in decoders - when they don't actually decode the whole image
(i.e. part of the memory is left uninitialized), but also don't report an issue.

Right now, MSAN seems to only detect uninitialized reads that are then
used (as conditional variable), so i'm [ab]using MSAN_MEM_IS_INITIALIZED().
This works great, and at this moment has already found ~18 issues.

However, it's not quite what i would like it to be.

  1. I need to manually plant MSAN_MEM_IS_INITIALIZED() calls.
  2. Deduplication on oss-fuzz is having problems with that Possibly better handling of MSAN reports oss-fuzz#863.
  3. I can't call it everywhere, it is simply unfeasible as it would significantly affect the code readability e.g.

So i'm wondering whether

  • there is already an option to instrument all reads? :) I guess not
  • if not, whether such functionality is in scope of MSAN, and could be added?

Roman.
@eugenis
Copy link
Contributor

eugenis commented Nov 20, 2017 via email

@LebedevRI
Copy link
Author

If you want to try, look at MemorySanitizerVisitor::visitLoadInst, and add
a call to insertShadowCheck().

Thank you for the reply, but i'm afraid that with my knowledge of compiler-rt, it is not enough info for me to do it.

@LebedevRI
Copy link
Author

Ok, so i looked at this once more.
https://github.com/llvm-mirror/llvm/blob/7f86494ddd7b6a32c98a0e8572c3ea96ac96f826/lib/Transforms/Instrumentation/MemorySanitizer.cpp#L1349-L1383

Given

int test(int* k) {
  return *k;
}

which is roughly

; Function Attrs: norecurse nounwind readonly uwtable
define i32 @test(int*)(i32* nocapture readonly) local_unnamed_addr #0 {
  %2 = load i32, i32* %0, align 4, !tbaa !2
  ret i32 %2
}

LoadInst &I is %2 = load i32, i32* %0, align 4

Now, int* k is %0, and is I.getPointerOperand();.
The shadow of the pointer is already checked in

if (ClCheckAccessAddress)
  insertShadowCheck(I.getPointerOperand(), &I);

Sidenote: why does it look like this check is after the load? https://godbolt.org/g/Mqbm85

So what we basically need to check is the shadow of the %2, i.e. the shadow of the [%0, %0+size) region. And this is where i'm loosing the idea.
My very naive idea was to do similar thing as in MS.TrackOrigins && PropagateShadow case:

{
  unsigned Alignment = I.getAlignment();
  unsigned OriginAlignment = std::max(kMinOriginAlignment, Alignment);

  insertShadowCheck(IRB.CreateAlignedLoad(getOriginPtr(Addr, IRB, Alignment),
                                          OriginAlignment),
                    &I);
}

but that crashes

clang-6.0: /build/llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1165: llvm::Value *(anonymous namespace)::MemorySanitizerVisitor::getShadow(llvm::Value *): Assertion `Shadow && "No shadow for a value"' failed.
#0 0x000000000241faf4 PrintStackTraceSignalHandler(void*) (/build/llvm-build-Clang-release/bin/clang-6.0+0x241faf4)
#1 0x000000000241fe56 SignalHandler(int) (/build/llvm-build-Clang-release/bin/clang-6.0+0x241fe56)
...
#8 0x00000000026527c8 (anonymous namespace)::MemorySanitizerVisitor::insertShadowCheck(llvm::Value*, llvm::Instruction*) (/build/llvm-build-Clang-release/bin/clang-6.0+0x26527c8)
#9 0x0000000002650d60 llvm::InstVisitor<(anonymous namespace)::MemorySanitizerVisitor, void>::visit(llvm::Instruction&) (/build/llvm-build-Clang-release/bin/clang-6.0+0x2650d60)

So i'm guessing that i'm passing something wrong as the first argument of the insertShadowCheck()

If there are any more hints, it might be interesting to make this work :)

@eugenis
Copy link
Contributor

eugenis commented Dec 5, 2017 via email

@LebedevRI
Copy link
Author

LebedevRI commented Dec 5, 2017 via email

@eugenis
Copy link
Contributor

eugenis commented Dec 5, 2017 via email

@morehouse
Copy link
Contributor

Do we want to add such a feature to MSan, or just leave this as experimental and close?

@LebedevRI
Copy link
Author

Personal opinion:
It would be interesting to have, especially in the strictest variant,
the questions are: maintainability and whether it regresses the performance
of the usual current non-strict variant.
It will obviously be much slower, not much to do there i would imagine.

@eugenis
Copy link
Contributor

eugenis commented Jun 8, 2018

The instrumentation itself is simple, but a proper implementation will require quite a bit of work - driver changes, function attributes (we'd want it enabled per-function and per-source file), tests, etc. And I don't expect it to be widely used.

I'd prefer not to have it in the codebase.

@LebedevRI
Copy link
Author

Would that add much maintenance burden?
Right now this sounds like one-time 'entry' cost, to do the proper integration,
which should be somewhat doable, maybe even for me.

LebedevRI added a commit to LebedevRI/rawspeed that referenced this issue Jul 4, 2018
@LebedevRI
ArwDecoder::GetWB(): round encrypted area down to multiple of 4.
77a1a1f 
And yet another issue that is detected by MSAN, but would be
so much more meaningful with
google/sanitizers#883
@LebedevRI LebedevRI mentioned this issue Aug 12, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@LebedevRI @eugenis @morehouse