[llvm-dev] Fixing some StackProtector issues

[via llvm-dev]

> -----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