llvm dev - Sep 2019 - Fixing some StackProtector issues

PR43308 describes a case where StackProtector fails to protect against
a fairly simple smash.  This problem started after r363169, which
removed StackProtector's own analysis function HasAddressTaken, and
used CaptureTracking's PointerMayBeCaptured instead.  The problem here
is that "pointer is captured" and "pointer could be used to smash
the
stack" are not equivalent queries.  The header of CaptureTracking.cpp
says in part:
  A pointer value is captured if the function makes a copy of any part
  of the pointer that outlives the call.
Clearly, stacks can be smashed without a "capture" occurring; a smash
simply uses a pointer in a normal-looking way, but just oversteps the
bounds of the pointed-to data.

r363169 was intended to fix PR42238, so undoing the patch will have to
come up with some other fix for PR42238.  Also, there's PR40436, which
identified an infinite recursion in HasAddressTaken; there's D57149 to
fix that, which was abandoned as irrelevant after r363169.

Finally, aside from just fixing PR42238, the commit log for r363169
also notes that it "fixes some infrastructure issues to allow running
just the IR pass."  That fix should be preserved.

Overall, I think the appropriate measures are:

1. Revert r363169, which fixes PR43308 but re-introduces PR42238 and
   PR40436.
2. Re-add the "infrastructure" fix from r363169.
3. Fix PR42238 by enhancing HasAddressTaken to look at new instructions.
4. Fix PR40436, by reviving D57149.

I'll start working on a patch series for the first 3 items; the
original author of D57149 can decide about reviving that one.

--paulr

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of via
> llvm-dev
> Sent: Thursday, September 19, 2019 10:37 AM
> To: llvm-dev at lists.llvm.org
> Subject: [llvm-dev] Fixing some StackProtector issues
> 
> PR43308 describes a case where StackProtector fails to protect against
> a fairly simple smash.  This problem started after r363169, which
> removed StackProtector's own analysis function HasAddressTaken, and
> used CaptureTracking's PointerMayBeCaptured instead.  The problem here
> is that "pointer is captured" and "pointer could be used to
smash the
> stack" are not equivalent queries.  The header of CaptureTracking.cpp
> says in part:
>   A pointer value is captured if the function makes a copy of any part
>   of the pointer that outlives the call.
> Clearly, stacks can be smashed without a "capture" occurring; a
smash
> simply uses a pointer in a normal-looking way, but just oversteps the
> bounds of the pointed-to data.
> 
> r363169 was intended to fix PR42238, so undoing the patch will have to
> come up with some other fix for PR42238.  Also, there's PR40436, which
> identified an infinite recursion in HasAddressTaken; there's D57149 to
> fix that, which was abandoned as irrelevant after r363169.
> 
> Finally, aside from just fixing PR42238, the commit log for r363169
> also notes that it "fixes some infrastructure issues to allow running
> just the IR pass."  That fix should be preserved.
> 
> Overall, I think the appropriate measures are:
> 
> 1. Revert r363169, which fixes PR43308 but re-introduces PR42238 and
>    PR40436.
See https://reviews.llvm.org/D67842
Ordinarily a revert wouldn't go through review, but as it's a base for
the other patches, I figured putting it on Phab couldn't hurt.  Also
it's two commits (the second was just a whitespace cleanup) making it
slightly awkward for reviewers to do themselves if they wish; this way
there's a single diff to download.
> 2. Re-add the "infrastructure" fix from r363169.
On second thought, no. This change was to allow running the StackProtector 
pass from opt; but it's a CodeGen pass, not a Transform, and it can already 
be run in isolation by llc. So this isn't needed.
> 3. Fix PR42238 by enhancing HasAddressTaken to look at new instructions.
See https://reviews.llvm.org/D67845 for a preliminary refactoring.
See https://reviews.llvm.org/D67846 for the functional change.

> 4. Fix PR40436, by reviving D57149.
> 
> I'll start working on a patch series for the first 3 items; the
> original author of D57149 can decide about reviving that one.
> 
> --paulr
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

> On Sep 21, 2019, at 19:22, via llvm-dev <llvm-dev at lists.llvm.org>
wrote:
> 
> On second thought, no. This change was to allow running the StackProtector 
> pass from opt; but it's a CodeGen pass, not a Transform, and it can
already
> be run in isolation by llc. So this isn't needed.
Not sure what you mean. llc can’t be used for running IR->IR passes

-Matt
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190921/d8f609af/attachment.html>