This is an archive of the discontinued LLVM Phabricator instance.

Implement pagerando wrapper functions to initialize POT register
Needs ReviewPublic

Authored by rinon on Sep 7 2017, 12:23 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Pagerando with PIP (Position Independent Pages) segments requires that all
externally visible (including address-taken) functions be wrapped with a
function that initializes the POT (Page Offset Table) register for use in the
protected PIP segment. Randomly located PIP segments cannot load the address of
the POT directly since that would require either being located at a fixed
PC-relative offset to a data segment or mutable code, both of which are not
acceptable options.

The PagerandoWrappersPass creates pagerando wrappers for all functions that
require a wrapper and marks all original functions with the PagerandoBinned
attribute so they will be placed into PIP bins when processing machine IR.

This patch set (D37580, D37581, D37582, D37583, D37584, D37585, D37586, D37587)
is a first draft of the pagerando implementation described in
http://lists.llvm.org/pipermail/llvm-dev/2017-June/113794.html.

Diff Detail

Event Timeline

rinon created this revision.Sep 7 2017, 12:23 PM

This patch set (D37580, D37581, D37582, D37583, D37584, D37585, D37586, D37587) is a first draft of the pagerando implementation described in http://lists.llvm.org/pipermail/llvm-dev/2017-June/113794.html.

small style nit: omit curly brackets for single-statement if/for/etc. statements.

lib/Transforms/IPO/PagerandoWrappers.cpp
78

Could you explain this condition?

130

What's the purpose of setting the Function section to the default here?

296
rinon added a comment.Sep 11 2017, 5:36 PM

Thanks for the comments. I'll fix the nits right away.

lib/Transforms/IPO/PagerandoWrappers.cpp
78

The last condition handles functions that consist of a single unreachable instruction. We don't every want to create a wrapper for these functions, since they shouldn't be emitted. I'll add a better comment here.

130

We want to assign these functions to unique section "bins" later in pagerando. Because we want to control the section that pagerando'd functions end up in to make sure it can be randomly placed, we don't respect the assigned section for the wrapped function. However, we do preserve any section assignment for the wrapper function that takes the given function's place (since we copy attributes (line 156).

rinon updated this revision to Diff 114741.Sep 11 2017, 6:22 PM
  • Fix style nits
rinon marked an inline comment as done.Sep 11 2017, 6:23 PM
nlewycky added inline comments.
include/llvm/IR/Attributes.td
202–206

I don't see a matching LangRef change.

I don't see a matching Verifier change. These are only applicable to functions (as opposed to return values or arguments)? Are these two attributes mutually exclusive?

lib/Transforms/IPO/PagerandoWrappers.cpp
61
78

That's not quite what this tests for. This would return true for a function that does work but ends in unreachable, say printing a fatal error message and calling abort. If you want to check for the 1-instruction function with unreachable, use isa<UnreachableInst>(&F.getEntryBlock().front()).

141

Please move the comment to before the code instead of using two-column code // comments.

151

Use llvm::Twine which is "a lightweight data structure for efficiently representing the concatenation of temporary values as strings." Value::setName takes a Twine argument. Something like:

F.setName(Twine(F.getName(), F.isVarArg() ? OrigVASuffix : OrigSuffix));
rinon updated this revision to Diff 116768.Sep 27 2017, 12:25 AM
  • Remove pagerando_wrapper attribute and rename pagerando_binned
  • Use SmallVector for address useses
  • Fix comment style
  • Do not pagerando trivial functions with only trap+unreachable
  • Use Twine for function name
rinon marked 3 inline comments as done.Sep 27 2017, 12:36 AM

Thanks for taking a look at this, Nick. I think this revision should address all comments so far.

include/llvm/IR/Attributes.td
202–206

I added a LangRef description.

After giving attributes a second look, I removed the PagerandoWrapper attribute as we don't need it at this point. The Pagerando attribute (formerly PagerandoBinned) is checked in the Verifier under isFuncOnlyAttr. I don't think we need to verify any other properties of the attribute, now that PagerandoWrapper is gone.

lib/Transforms/IPO/PagerandoWrappers.cpp
78

Good point. I intended to catch functions like the following (generated for abstract, non-base destructors):

call void @llvm.dbg.value...
tail call void @llvm.trap()
unreachable

Randomizing the page address of such functions is pointless, so we can save code size by not enabling pagerando for functions like these and thus not need wrappers. I've now made this optimization more explicit. If you think we should just leave this out for the sake of simplicity, I'm fine with that as well.

rinon updated this revision to Diff 118511.Oct 10 2017, 5:42 PM
  • Rebase
  • Don't apply pagerando to naked functions

A naked function should not get a pagerando wrapper, so cannot be placed in a
bin if it would require a wrapper. Naked functions are also used for CFI jump
tables (on non-windows platforms), which cannot be placed in pagerando bins
because they are PC-relative direct jumps in inline ASM.

peter.smith added inline comments.
docs/LangRef.rst
1611

Is section the right word to use here? The compiler will place the function in a section, the linker will place that in an output section that will presumably be described, at least in ELF terms as a segment.

lib/Transforms/IPO/PagerandoWrappers.cpp
102

Are the issues surrounding comdat that at code-generation time that we can't guarantee at link time which comdat group is selected and it may not be the one with wrappers? If I'm write it would be useful to know, although not necessarily in this review, what ideas you have to resolve this?

rinon edited the summary of this revision. (Show Details)Oct 31 2017, 11:42 AM
rinon added inline comments.Oct 31 2017, 5:01 PM
docs/LangRef.rst
1611

I'm open to either section or segment. I wasn't sure which one made more sense, since the compiler only cares about sections, and in this case the section and segment need to be the same.

lib/Transforms/IPO/PagerandoWrappers.cpp
102

Not quite. We don't really care if non-pagerando comdat groups are selected since this implementation should only refer to wrapped, external functions and cannot call functions in pagerando bins directly. To make sure that the wrappers are selected if their corresponding implementations are selected, wrappers need to go in the same comdat group as their implementation function, which is easy. We would need to treat all comdat functions as externally visible and potentially replaced to handle the case where the external implementation is selected.

However, if we mix comdat groups and pagerando bins, it turns out that we cannot control the layout of the resulting section precisely. The linker may (and in my testing did) reorder resolved comdat sections when merging them into our named output section (pagerando bin), which ruins the section offsets we computed during code generation for referencing binned functions.

Looking closer at the gold plugin interface specifically, the gold documentation seems to promise that all comdat groups will be resolved to the IR input file version, and no comdat symbols should be in the LTO output file (see COMDAT Sections in https://gcc.gnu.org/wiki/whopr/driver). So, as long as we're doing full LTO with gold, we should be able to resolve comdat before assigning to bins and the whole issue goes away. However, as far as I can tell, LLVM doesn't actually do that right now. I built some simple C++ with full (gold) LTO, saved temps, and the LTO output still included COMDAT sections.

Basically, I think we either need cooperation from the linker in how comdat sections are merged together, or we need to resolve and remove comdat during LTO. Alternatively, if we add linker support for new relocations (section->symbol offset and POT entry index), I think this issue goes away, but that is more involved and requires upstreaming to multiple projects.

rinon updated this revision to Diff 130534.Jan 18 2018, 6:29 PM

Ensure that pagerando wrapper functions are placed before binned functions

Gold assumes that the first executable section should be laid out first in the
file, but we want the normal .text section to come first and all pagerando bins
to be placed at the end of the file. To ensure that the first function emitted
will be in the normal .text section we prepend wrapper functions (which are
emitted to the normal .text) onto the Module function list.

rinon updated this revision to Diff 131174.Jan 23 2018, 5:22 PM

Explicitly propagate applicable function attributes to pagerando wrappers.

rinon updated this revision to Diff 131296.Jan 24 2018, 9:41 AM
  • Do not wrap thunk functions
  • Restore propagation of function attributes not in attribute list

Attributes are now propagated by default to pagerando wrappers and attributes
that do not apply to wrappers are blacklisted explicitly.

rinon updated this revision to Diff 140705.Apr 2 2018, 4:32 PM
  • Only wrap functions marked with pagerando attribute.

    The frontend should now explicitly opt in to pagerando with a function attribute, allowing control of pagerando on a per-function granularity via the existing sanitizer flag infrastructure.
  • Refactor replacement of function references with wrappers
  • Don't skip aliases when determining pagerando wrappers
  • Don't apply pagerando to trivial functions

    Wrapping trivial (single forwarding call) functions results in a large proportional increase in overhead for these functions. Not applying pagerando in this case results in effectively equivalent security, assuming that the trivial function does not contain useful gadgets, since an attacker could reuse the whole function. This optimization can be enabled via a command-line option.
  • Rebase
hintonda removed a subscriber: hintonda.Apr 4 2018, 4:39 PM
rinon updated this revision to Diff 156642.Jul 20 2018, 4:56 PM

Rebase

  • Update pagerando wrapper tests