[PATCH] D108457: [hwasan] Do not instrument accesses to uninteresting allocas.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added a subscriber: vitalybuka.
eugenis added a comment.

@vitalybuka

Ideas for more analysis tests:

- unsafe alloca with a mix of safe and unsafe accesses
- memcpy that is safe on one side and unsafe on the other. Either between two allocas, or within the same (memmove?). Or between alloca and non-stack memory.



In D108457#2985198 <https://reviews.llvm.org/D108457#2985198>, @fmayer wrote:

> In D108457#2984855 <https://reviews.llvm.org/D108457#2984855>, @fmayer wrote:
>
>> In D108457#2983236 <https://reviews.llvm.org/D108457#2983236>, @eugenis wrote:
>>
>>> What if an instruction may access either stack or heap?
>>>
>>>   i32 *p = flag ? p_heap_i16 : &stack_i32;
>>>   *p = 42;
>>>
>>> The analysis will say "safe" because it is only scanning from the stack roots.
>>> This should probably be fixed in hwasan by tracking the underlying alloca.
>>
>> Ah yes, I did handle this but then accidentally lost that when I refactored around some stuff. Put that back and added an IR test.
>
> Thinking again I remembered why I removed the explicit case for this during the refactoring: in this case, SCEV will not be able to calculate an in-range offset between the operant of the store and the alloca, so it will not be judged a safe access

Right, of course. Any reachable instruction we would require to be *always* within the alloca range.



================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:836
         }
+        for (const auto &A : KV.second.Accesses) {
+          bool IsSafe = AIRange.contains(A.second);
----------------
This affect compilation time and memory. Ideally we would not do it if the client can not use this info (ex. MTE).


================
Comment at: llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp:787
+    const AllocaInst *AI = findAllocaForValue(Ptr);
+    if (AI)
+      return true;
----------------
`if (findAllocaForValue(Ptr)) return true;`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108457