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

[Magee, Joshua]

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: part3-sspstrong-datalayout-v2.diff
Type: application/octet-stream
Size: 84389 bytes
Desc: part3-sspstrong-datalayout-v2.diff
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130410/20712b38/attachment.obj>