[PATCH] -fstack-protector-strong part 3: data layout

[Evan Cheng]

Hi Josh,

A few more questions and concerns:

1. Is the new behavior opted-in?
2. Did you measure the compile time impact?
3. Do you know why the stack protector pass requires dominator information?
4. I am slightly concerned about the change to MFI->CreateStackObject(). This may impact out-of-tree targets. If this patch goes in, please send a message to llvmdev to inform folks of the internal API change.

Since this is a lot of code, we need someone who is familiar with the stack protector implementation to go through the patch carefully. Bill, are you the right person to do this?

Evan

On May 15, 2013, at 7:29 PM, "Magee, Joshua" <Joshua_Magee at playstation.sony.com> wrote:

> Ping, with updated patch against top-of-tree.
> 
> 
> - Josh
> 
> 
> At 1365663271 seconds past the Epoch, Magee, Joshua wrote:
>> Attached is a re-work of the SSP data layout patch I submitted back in
>> February.  (Sorry about the extremely tardy follow-up.)
>> 
>> This version removes the introduction of a field to AllocaInst (which was used
>> to communicate the layout preference of an AllocaInst from the StackProtector
>> pass to the PEI/LocalStack passes.)  Instead the SSPLayoutKind required for each
>> AI is communicated with getAnalysis().  Currently, the analysis is implemented
>> as part of the StackProtector pass.  Another possibility would be to split the
>> analysis into a separate pass thereby yielding 1 mutation-free analysis pass
>> (which determines which AllocaInsts trigger protectors and the SSPLayoutKind they
>> require) and another transformation pass (which actually does the insertion of
>> the protectors.)  I decided to leave it as one pass for now and then see if
>> anyone expressed preference for the other during review.
>> 
>> One thing that I am not happy is that I needed to mark several other passes as
>> preserving the stack-protector analysis.  It felt like there should be a better
>> way to express that the analysis won't be invalidated by subsequent passes, but
>> I couldn't find one.
>> 
>> - Josh
>> 
>> 
>>> -----Original Message-----
>>> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
>>> bounces at cs.uiuc.edu] On Behalf Of Magee, Josh
>>> Sent: Tuesday, February 12, 2013 4:45 PM
>>> To: Bill Wendling
>>> Cc: llvm-commits at cs.uiuc.edu
>>> Subject: RE: [PATCH] -fstack-protector-strong part 3: data layout
>>> 
>>> Hi Bill,
>>> 
>>> I had wondered if adding a field to AllocaInst might be a problem (especially
>>> since the field is for a fairly specialized purpose that is not relevant to the
>>> majority of users of the class.)  I'll re-work the patch to avoid modifying
>>> AllocaInst.  (My current idea is to maintain a ValueMap of <AllocaInst*,
>>> SSPLayoutKinds> in the StackProtector pass itself and then use getAnalysis
>>> to access this from the PEI and  LocalStackAllocation passes.  It seems like I
>>> might need to split the StackProtector pass into separate analysis and
>>> transform passes, but I'm not certain yet.)
>>> 
>>> Hopefully I can have an update ready within the next couple of days.
>>> Thanks!
>>> Josh
>>> 
>>> 
>>>> -----Original Message-----
>>>> From: Bill Wendling [mailto:wendling at apple.com]
>>>> Sent: Tuesday, February 12, 2013 12:31 AM
>>>> To: Magee, Josh
>>>> Cc: llvm-commits at cs.uiuc.edu
>>>> Subject: Re: [PATCH] -fstack-protector-strong part 3: data layout
>>>> 
>>>> Hi Josh,
>>>> 
>>>> I'll review this in a few days (sorry, I'm swamped). One comment
>>>> though, is there *any* way for you to not add a field to AllocaInst?
>>>> I'm going to push back hard on that because it's a basic class and
>>>> shouldn't be modified unless absolutely necessary, especially if
>>>> you're adding fields to it. :)
>>>> 
>>>> -bw
>>>> 
>>>> On Feb 11, 2013, at 7:01 PM, "Magee, Josh"
>>> <Joshua.Magee at am.sony.com>
>>>> wrote:
>>>> 
>>>>> Hi,
>>>>> 
>>>>> This is part 3 in the stack-protector-strong series.
>>>>> This patch adds support for data layout rules.
>>>>> 
>>>>> The rules for ssp are:
>>>>> 1) Large arrays and structures containing large arrays should be closest to
>>>>>   the stack protector.  "Large" is defined as >= ssp-buffer-size.
>>>>> 2) The remaining stack objects are arranged normally.
>>>>> 
>>>>> These rules for sspstrong and sspreq are the same:
>>>>> 1) Large arrays and structures containing large arrays should be closest to
>>>>>   the stack protector.  "Large" is defined as >= ssp-buffer-size.
>>>>> 2) Small arrays and structures containing small arrays should be 2nd
>>>>> closet
>>>> to
>>>>>   the protector.  "Small" is defined as < ssp-buffer-size.
>>>>> 3) Variables that have had their address taken should be 3rd closest to
>>> the
>>>>>   protector.
>>>>> 4) The remaining stack objects are arranged normally.
>>>>> 
>>>>> LLVM already allocates SSP-triggering objects near the stack protector,
>>> but:
>>>>> a) The heuristic used is very basic and doesn't necessarily match what
>>> the
>>>>>   StackProtector pass does.
>>>>> b) It doesn't handle the finer grained rules for strong/req listed above.
>>>>> 
>>>>> Rather than determining if a stack object may need to be placed near
>>>>> the protector in FunctionLoweringInfo.cpp, I thought it is better to
>>>>> make this decision in the StackProtector pass (where we already do
>>>>> the needed analysis
>>>>> anyway.)
>>>>> 
>>>>> The changes I made are:
>>>>> 1) Add an enum type to represent the SSP Layout.
>>>>>  * SSPLayoutKind
>>>>> 2) Add a field to AllocaInst to keep track of the SSPLayoutKind.
>>>>> 3) Update the StackProtector pass analysis to set the SSPLayoutKind for
>>> an
>>>>>   AllocaInst.  This requires two unfortunate changes:
>>>>>  * Since all variables vulnerable to stack smashing need to be re-
>>> arranged
>>>>>    (and not just the first one encountered), the analyze can't exit early
>>>>>    after finding the first one.
>>>>>  * The analysis needs to be run for SSPReq.  Before it wasn't necessary
>>> for
>>>>>    SSPReq because all SSPReq functions get a protector regardless of their
>>>>>    variables. Now the analysis is needed to mark the vulnerable
>>> variables.
>>>>> 4) Teach the PrologEpilogInserter and LocalStackAllocation passes
>>>>> the new
>>>> SSP
>>>>>   stack re-arrangement rules.
>>>>> 
>>>>> Thanks!
>>>>> -Josh
>>>>> <part3-sspstrong-datalayout.diff>
>>>> 
>>> 
>>> 
>>> 
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> 
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> <part3-sspstrong-datalayout-v2_2.diff>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits