Skip to content

Discuss control-flow integrity implementation #723

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 12, 2017

Conversation

ddcc
Copy link
Contributor

@ddcc ddcc commented Jul 12, 2016

This is the follow-up to #717 that discusses fine-grained CFI for WebAssembly, currently implemented using runtime instrumentation from Clang/LLVM, but proposed to use multiple tables.

@ddcc
Copy link
Contributor Author

ddcc commented Jul 12, 2016

It's not ready to be merged, but I'd like to get some feedback on the proposal. I plan to implement multiple table support and perform some benchmarks to measure the code size decrease (remove of runtime instrumentation), increase in runtime speed, etc.


Support for fine-grained control-flow integrity in WebAssembly operates as
follows. First, the Clang frontend emits type signatures for each function
and each call site in the LLVM IR. This is used to partition all accessible
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually wasm 'supporting' CFI, or one implementation built on wasm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from the coarse-grained CFI that we have already, it is supporting a fine-grained CFI implementation from Clang/LLVM.

@d1m0
Copy link

d1m0 commented Jul 12, 2016

I am trying to understand how fine-grained a CFI scheme we are aiming to support using Wasm primitives. Specifically do we support non-disjoint sets? I think the referenced LLVM CFI implementation does support them, but the proposal talks about disjoint sets.

To make the question concrete, here is a small example: Imagine a C++ program with classes A,B,C,D all implementing/overriding a method f(). They form the following hierarchy:

      A
     /  \
   B    C
   |
   D

And we have the following program:

        A* a = ... ;
(1)     a->f(); // safe values are Af,Bf,Cf,Df
        B *b = ...;
(2)     b->f(); // safe values are Bf, Df

Are the allowed sets of targets at (1) and (2) the same? If not how would this be enforced in wasm?

If we want to support different sets for (1) and (2) in Wasm, using separate function tables, it seems like there would be a lot of duplication.

@ddcc
Copy link
Contributor Author

ddcc commented Jul 13, 2016

There are no limits on CFI granularity from Wasm itself, but efficiency of the CFI implementation tends to be a usage concern.

The Clang/LLVM implementation doesn't support disjoint sets, because it uses type signatures. So, in your example, all implementations of f() would be in the same set. Adding support for non-disjoint sets is practically infeasible with a single homogeneous table and index range checking. One could implement partial support by allowing overlapping ranges, but that requires careful layout of indexes, and doesn't scale well. At some point, it would need instrumentation for checking multiple independent ranges, which has high runtime overhead.

Nevertheless, non-disjoint tables should be easily doable using multiple homogeneous tables, although you do end up with duplicate entries. From your example, put (1) in one table, and (2) in another table. Of course, you could optimize this further with some sort of hybrid multiple tables plus range-based checking approach, but that's beyond the scope of this proposal.

Thanks, I will update the text to clarify based on the comments.

@ddcc
Copy link
Contributor Author

ddcc commented Aug 20, 2016

I've updated the text, please take a look. The single table CFI implementation has also been merged into upstream Clang/LLVM.

@ddcc ddcc force-pushed the cfi branch 2 times, most recently from bb0daa3 to 69f73a9 Compare August 20, 2016 03:21
@dschuff dschuff removed this from the Future Features milestone Aug 26, 2016
@dschuff
Copy link
Member

dschuff commented Aug 26, 2016

Since this now focuses on current features, it doesn't need to have the Future Features milestone. :)

@ddcc
Copy link
Contributor Author

ddcc commented Oct 31, 2016

Rebased

@jfbastien
Copy link
Member

This seems non-controversial based on comments and how long it's been open. I've seen people ask about CFI elsewhere, so even if it isn't comprehensive it's a step in the right direction. Therefore, merging, happy to take more PRs to improve.

@jfbastien jfbastien merged commit dea1548 into WebAssembly:master May 12, 2017
@ddcc
Copy link
Contributor Author

ddcc commented May 12, 2017

Good to see this finally getting merged :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants