[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl created this revision.
Herald added subscribers: cfe-commits, jdoerfert, mgorny.
Herald added a project: clang.

This patch set introduces structure field layout randomization into the Clang
compiler. The Randstruct feature is a compile-time hardening technique that 
randomizes the field layout for designated structures of a code base. 
Admittedly, this is mostly useful for closed-source releases of code (since 
the randomization seed would be available for public and open source application
s). However, this patch set also enhances Clang’s feature parity with that 
of GCC which already has the Randstruct feature.

This patch set is a from-scratch reimplementation of the Randstruct feature 
that was originally ported to GCC. The patches for this implementation in GCC 
can be found here:

  https://www.openwall.com/lists/kernel-hardening/2017/04/06/14.

This feature identifies structures for randomization in two ways. The first 
method targets structures that are manually marked with the new 
“randomize_layout” attribute. The second is an optional feature that will 
automatically select and randomize structures that are found to consist entirely
of function pointers. This automatic selection feature can be extended to 
include other vulnerable structure types that are safe to randomize as they are
identified. You can also opt a specific structure out of this feature with the 
“no_randomize_layout” attribute. Automatic structure selection is enabled with 
the “-randstruct-auto” compiler flag. By default, Randstruct seeds on the empty
string, but a seed can be supplied with the “-randstruct-seed=” command line 
argument.

This entire patch set is the sum total of an undergraduate computer science 
capstone team’s effort.

Portland State University Clang Randstruct Capstone Team (Fall 2018-Winter 
2019):

Co-authored-by: Cole Nixon 
Co-authored-by: Connor Kuehl 
Co-authored-by: James Foster 
Co-authored-by: Jeff Takahashi 
Co-authored-by: Jordan Cantrell 
Co-authored-by: Nikk Forbus 
Co-authored-by: Tim Pugh 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D59254

Files:
  clang/include/clang/AST/Decl.h
  clang/include/clang/AST/DeclBase.h
  clang/include/clang/AST/RandstructSeed.h
  clang/include/clang/AST/RecordFieldReorganizer.h
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticASTKinds.td
  clang/include/clang/Driver/CC1Options.td
  clang/include/clang/Driver/Options.td
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/DeclBase.cpp
  clang/lib/AST/RecordFieldReorganizer.cpp
  clang/lib/AST/RecordLayoutBuilder.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test

Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -81,6 +81,7 @@
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function)
 // CHECK-NEXT: NoMicroMips (SubjectMatchRule_function)
 // CHECK-NEXT: NoMips16 (SubjectMatchRule_function)
+// CHECK-NEXT: NoRandomizeLayout (SubjectMatchRule_record)
 // CHECK-NEXT: NoSanitize (SubjectMatchRule_function, SubjectMatchRule_objc_method, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSanitizeSpecific (SubjectMatchRule_function, SubjectMatchRule_variable_is_global)
 // CHECK-NEXT: NoSpeculativeLoadHardening (SubjectMatchRule_function, SubjectMatchRule_objc_method)
@@ -118,6 +119,7 @@
 // CHECK-NEXT: Overloadable (SubjectMatchRule_function)
 // CHECK-NEXT: ParamTypestate (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: PassObjectSize (SubjectMatchRule_variable_is_parameter)
+// CHECK-NEXT: RandomizeLayout (SubjectMatchRule_record)
 // CHECK-NEXT: RenderScriptKernel (SubjectMatchRule_function)
 // CHECK-NEXT: ReqdWorkGroupSize (SubjectMatchRule_function)
 // CHECK-NEXT: RequireConstantInit (SubjectMatchRule_variable_is_global)
Index: clang/lib/Sema/SemaDeclAttr.cpp
===
--- clang/lib/Sema/SemaDeclAttr.cpp
+++ clang/lib/Sema/SemaDeclAttr.cpp
@@ -6966,6 +6966,12 @@
 handleSimpleAttributeWithExclusions(S, D, AL);
 break;
+  case ParsedAttr::AT_RandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
+  case ParsedAttr::AT_NoRandomizeLayout:
+handleSimpleAttribute(S, D, AL);
+break;
   case ParsedAttr::AT_CodeSeg:
 handleCodeSegAttr(S, D, AL);
 break;
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -6,6 +6,7 @@
 //
 //===--===//

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added inline comments.



Comment at: clang/lib/AST/DeclBase.cpp:1262
+  // The last one in the chain should have a null next!
+  PrevDecl->NextInContextAndBits.setPointer(nullptr);
+

Apologies that this is included. I've left a comment on 
`clang/lib/AST/RecordFieldReorganizer.cpp` describing our issues with this 
code. I also mention experimenting with fixing it in that comment as well, and 
in those experiments we've been able to remove this change from BuildDeclChain. 
We're just waiting for further advice before we change the code too rapidly.



Comment at: clang/lib/AST/RecordFieldReorganizer.cpp:58
+  Decl *First, *Last;
+  std::tie(First, Last) = DeclContext::BuildDeclChain(
+  NewFieldOrder, D->hasLoadedFieldsFromExternalStorage());

This part of the code where we rebuild the new DeclChain with the new order of 
fields is resulting in a Clang compiler that segfaults when building the Linux 
kernel.

Any advice with how we can safely manage the DeclContext field chain is greatly 
appreciated here.

In our experimentation (which isn't present in this diff here) we thought our 
best bet to was to save the LastDecl's next pointer before BuildDeclChain is 
called and then we set the NEW LastDecl's next pointer to it in an effort to 
preserve whatever context chaining is happening.

Truthfully, we don't fully understand the machinery of the DeclContext or why 
our assumptions about this linked list of fields are incorrect.

More troubleshooting information regarding this particular instance is on our 
github issue: https://github.com/clang-randstruct/llvm-project/issues/42







Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added a comment.

In D59254#1426809 , @pcc wrote:

> This needs a test under `test/CodeGen` at least.


Will do, thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added inline comments.



Comment at: clang/include/clang/AST/RecordFieldReorganizer.h:54
+  std::seed_seq Seq;
+  std::default_random_engine rng;
+};

pcc wrote:
> I don't think we can use `default_random_engine` for this because the 
> behaviour would need to be consistent between C++ standard library 
> implementations, and the behaviour of `default_random_engine` is 
> implementation defined. Similarly, I don't think that we can use 
> `std::shuffle` (see note in 
> https://en.cppreference.com/w/cpp/algorithm/random_shuffle ).
Sure thing, we'll begin investigating alternatives.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added a comment.

In D59254#1429401 , @jfb wrote:

> I find it easier to understand the code by looking at the tests. When you add 
> tests, please make sure you test for:
>
> - Bit-fields
> - Zero-width bit-field


Hi JF,

Could you elaborate on what I should be testing for here regarding the 
zero-width bit-field? I'm not sure zero-width bit-fields are a concern since I 
think they're illegal. I just tried compiling a "hello world" program with a 
struct that has a zero width bit-field as one of its members and the compiler 
emitted an error pointing at it.

  error: named bit-field 'z' has zero width
  int z : 0;
  ^
  1 error generated.

For regular bit-fields the current implementation will keep adjacent bit-fields 
together and will preserve their original order but the overall group of 
adjacent bit-fields may relocate. My next revision will have a unit test 
describing and testing this behavior.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added a comment.

In D59254#1565961 , @jfb wrote:

> Do you intend on moving this forward? It seems like a Good Thing overall, and 
> I'd love to help review some more.


Yes. I'm sorry it's been in limbo so long. I haven't touched base with the rest 
of my team so I can't speak for them, but I'm interested in moving this along. 
I've been busy settling into work and post-university life lately.

From your initial review it sounds like there's quite a bit to be done by way 
of providing sufficient test coverage for this and some code cleanup. A few 
other kind contributors have left us advice on our github repo for some 
blocking issues with designated initializers.

I'll be reviewing all the advice we've been given here and on our repo and 
figuring out the best way to start making progress on this again. There's 
definitely a good basis here.

That said, I'm still interested in and would appreciate any advice or pointers 
to resources. I don't suppose there's an LLVM/Clang mentorship type program?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added a comment.

In D59254#3298184 , @void wrote:

> In D59254#1792134 , @connorkuehl 
> wrote:
>
>> In D59254#1792068 , @xbolva00 wrote:
>>
>>> Re-ping
>>
>> Still under development here: 
>> https://github.com/connorkuehl/llvm-project/tree/randstruct
>
> Hi @connorkuehl,
>
> I was asked by @kees to take a look at implementing this, with the view to 
> support it in the Linux kernel. There doesn't seem to have been a lot of work 
> on this recently. Is it effectively in limbo at the moment?

@void Cool! Yeah, I haven't done anything with this. The git tree in the quoted 
comment (https://github.com/connorkuehl/llvm-project/tree/randstruct) does have 
a lot more progress compared to this Phabricator submission in terms of some 
bug fixes and unit test coverage, though there's still room for improvement in 
terms of performance (and possibly correctness, iirc compiling Linux segfaulted 
Clang with those patches). Furthermore, the patchset is quite old, so I would 
be entirely shocked if it applied without much fuss on a recent LLVM tree. As 
you can see, the latest commit was in late 2019.

Best of luck! I hope at least some of it is helpful to you.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D59254: [RFC] Implementation of Clang randstruct

[Connor Kuehl via Phabricator via cfe-commits]

connorkuehl added a comment.

In D59254#1792068 , @xbolva00 wrote:

> Re-ping


Still under development here: 
https://github.com/connorkuehl/llvm-project/tree/randstruct


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59254/new/

https://reviews.llvm.org/D59254



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits