[PATCH] D109816: [hwasan] also omit safe mem[cpy|mov|set].

Evgenii Stepanov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 20 13:59:14 PDT 2021


eugenis added a subscriber: vitalybuka.
eugenis added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:800
       return true;
-  } else if (SSI && SSI->accessIsSafe(*Inst)) {
+  } else if (SSI && SSI->accessIsSafe(*Inst) && findAllocaForValue(Ptr)) {
+    // Because SSI->accessIsSafe only tells us that all *stack* accesses in
----------------
Now since both branches do interesting stuff only when findAllocaForValue is non-null, we can hoist this predicate.

```
if (findAllocaForValue(Ptr)) {
  if (!InstrumentedStack) return true;
  if ((SSI && SSI->accessIsSafe(*Inst)) return true;
}
```




================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:789
       return true;
-  } else if (SSI && SSI->accessIsSafe(*Inst)) {
+  } else if (SSI && SSI->accessIsSafe(*Inst) && findAllocaForValue(Ptr)) {
     return true;
----------------
fmayer wrote:
> eugenis wrote:
> > fmayer wrote:
> > > eugenis wrote:
> > > > Is this a separate bugfix?
> > > > 
> > > > Am I right that this is not needed for regular load/store because the argument is required to be 100% traceable to a single alloca, but 2-args memintrinsics are safe if one arg is 100%, and the other is 100% not stack? That does not sound right.
> > > > 
> > > > The comment on accessIsSafe does not even try to cover such cases. Also, I do not see any tests under Analysis/StackSafety for the mixed memintrinsic case.
> > > Before this change, we only checked this for instructions that took a single pointer variable. There were two cases in `accessIsSafe`:
> > > 
> > > * the pointer is non stack: we never reached this instruction in the stack safety pass, and we return false because we do not find it in the mapping.
> > > * the pointer is potentially stack: we reach this instruction in the pass, and SECV will take care of checking whether we can *only* reach it from the alloca.
> > > 
> > > Now, for memcpy / memmove there is the extra case that we can have both cases for two arguments. Now the problem becomes if one argument is in range of the alloca, but the other one isn't reachable from any alloca, accessIsSafe will return true.
> > > 
> > > What I did in this change is clarify the semantics (as they already were) of accessIsSafe in the comment, and defer responsibility to check whether *all* arguments are potentially stack to the caller. As such, we only hit the second case above.
> > Yeah, I agree with that. I just want it clarified even more.
> > 
> > For a sample of cases:
> > 1. dest and source both are always stack and safe
> > 2. dest is always stack and safe; source is never stack
> > 3. dest is either stack and safe or not stack; source is never stack
> > 
> > the comment does not give me confidence in what isAccessSafe answer would be.
> > 
> I clarified the comment and added some more tests.
I still find the description confusing, especially around "potential stack accesses".

How about this:
* rename to stackAccessIsSafe
* describe approx. like this: "returns true if the instruction can be proven to do only two types of memory accesses (1) live stack locations in-bounds or (2) non-stack locations"

i.e. not "accesses that may go to stack are safe" but "safe when goes to stack".
And this makes it clear that predicating the check with findAllocaForValue() makes it indeed 100% safe.

To be true to this description, the function should return true if the argument can not be traced back to an alloca. I think this can be implemented by removing "SafeAccesses" and looking straight at "AccessIsUnsafe".

WDYT?
@vitalybuka 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D109816/new/

https://reviews.llvm.org/D109816



More information about the llvm-commits mailing list