[LLVMdev] load widening conflicts with AddressSanitizer

[Kostya Serebryany]

Hi,
[resurrecting an old mail thread about AddressSanitizer false positive
caused by load widening]

Once the Attribute::AddressSafety is set by clang (a separate patch),
fixing this bug may look as simple as this:

--- lib/Analysis/MemoryDependenceAnalysis.cpp   (revision 148708)
+++ lib/Analysis/MemoryDependenceAnalysis.cpp   (working copy)
@@ -323,6 +323,14 @@
         !TD.fitsInLegalInteger(NewLoadByteSize*8))
       return 0;

+    if (LI->getParent()->getParent()->hasFnAttr(Attribute::AddressSafety)
&&
+        LIOffs+NewLoadByteSize > MemLocEnd) {
+      // We will be reading past the location accessed by the original
program.
+      // While this is safe in a regular build, Address Safety analysys
tools
+      // may start reporting false warnings. So, do't do widening.
+      return 0;
+    }
+
     // If a load of this width would include all of MemLoc, then we
succeed.
     if (LIOffs+NewLoadByteSize >= MemLocEnd)
       return NewLoadByteSize;


Other suggestions, objections?

Thanks,
--kcc





On Wed, Dec 28, 2011 at 1:12 PM, Criswell, John T <criswell at illinois.edu>wrote:

>
>  ------------------------------
> *From:* Kostya Serebryany [kcc at google.com]
> *Sent:* Wednesday, December 28, 2011 2:46 PM
> *To:* Criswell, John T
>
> *Cc:* llvmdev at cs.uiuc.edu
> *Subject:* Re: [LLVMdev] load widening conflicts with AddressSanitizer
>
>
>
> On Wed, Dec 28, 2011 at 12:40 PM, Criswell, John T <criswell at illinois.edu>wrote:
>
>>  Dear All,
>>
>>  I think adding metadata and expecting transforms to repect it is a bad
>> idea.  It is just too easy for someone who does not know about the metadata
>> to add a transform that ignores it.
>>
>>  As for SAFECode, I think we have one of several options for handling
>> load-widening.  The most obvious one is to have a pass that just boosts the
>> allocation size of any alloca with an align 16 attribute;
>>
>
>  This may lead to real bugs being lost (false negatives).
>
>  I believe it would remove memory unsafe behavior from the program (so
> the program is still functionally incorrect, but it would not violate
> memory safety, making memory safety attacks impossible).
>
>
>
>
>
>>  this pass would only be scheduled to execute when the other SAFECode
>> instrumentation passes are scheduled to execute.  Another option would be
>> to just disable the load-widening transform or to use a specialized version
>> that only widens when the allocation size does not cause a problem.
>>
>
>  To disable load widening you need to pass some flag to the load widening
> phase.
> Passing it through metadata is one of the possible solutions.
>
>  Or we can disable load widening from the clang driver, but then we will
> need a flag for that (do we have it now?)
>
>  If you have modified the clang driver with a -fasn option, then you
> could also modify the driver so that it does not run the load-widening pass
> when the -fasn option is given on the command-line.
>
>  This assumes, of course, that the load-widening transform is not part of
> some other transform (like instcombine).  If it is part of another
> transform, it would be split out into its own transform (separation of
> concerns and all that).
>
>  -- John T.
>
>
>  --kcc
>
>
>>
>>  -- John T.
>>
>>  ------------------------------
>> *From:* llvmdev-bounces at cs.uiuc.edu [llvmdev-bounces at cs.uiuc.edu] on
>> behalf of Kostya Serebryany [kcc at google.com]
>> *Sent:* Tuesday, December 27, 2011 12:57 PM
>> *To:* Chris Lattner
>> *Cc:* llvmdev at cs.uiuc.edu
>> *Subject:* Re: [LLVMdev] load widening conflicts with AddressSanitizer
>>
>>
>>
>> On Mon, Dec 19, 2011 at 4:27 PM, Chris Lattner <clattner at apple.com>wrote:
>>
>>>
>>> On Dec 17, 2011, at 7:40 AM, Rafael Ávila de Espíndola wrote:
>>>
>>> > On 16/12/11 08:46 PM, Chris Lattner wrote:
>>> >> I'm not opposed to disabling this transformation when asan is on, we
>>> just need a clean way to express this in the IR.
>>> >
>>> > Could clang be aware of asan being on and introduce a "please don't
>>> > widen" metadata on local variable accesses?
>>>
>>>  Yes, "we just need a clean way to express this in the IR."
>>>
>>> LLVM can't have a global "bool ASANIsOn;" that the optimizers listen to.
>>>
>>
>>
>>  A global is bad.
>> What about a metadata attached to a Function saying that transformations
>> which will read out of bounds (even "safely") are illegal?
>> asan and SAFEcode will add this metadata, optimizers will listen to it.
>>
>>  Any other suggestion?
>>
>>
>>  --kcc
>>
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20120123/0f1f7ffc/attachment.html>