Page MenuHomePhabricator

Add support for __declspec(guard(nocf))
ClosedPublic

Authored by ajpaverd on Jan 3 2020, 9:15 AM.

Details

Summary

Avoid using the nocf_check attribute with Control Flow Guard. Instead, use a
new "guard_nocf" function attribute to indicate that checks should not be
added on indirect calls within that function. Add support for
__declspec(guard(nocf)) following the same syntax as MSVC.

Diff Detail

Event Timeline

ajpaverd created this revision.Jan 3 2020, 9:15 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 3 2020, 9:15 AM

Thanks for doing this!

When I applied this patch and added __declspec(guard(nocf)) to the function that was giving us trouble, I still crashed with a CFG failure in the caller, since the nocf function was inlined. When I additionally marked that function as noinline, then the CFG checks were omitted, as desired. Is this behavior with respect to inlining expected?

You may want to mention "PR44096" in the commit message.

Can you please add some Sema tests that verify the attribute only appertains to functions, accepts the right number of arguments, etc? Also, some tests showing how this attribute works for things like redelcarations and virtual functions would help. e.g.,

void f();
__declspec(guard(nocf)) void f() {} // Is this fine?

__declspec(guard(nocf)) void g();
void g() {} // How about this?

struct S {
  __declspec(guard(nocf)) virtual void f();
};

struct T : S {
  void f() override; // Should this be guard(nocf) as well?
};
clang/include/clang/Basic/AttrDocs.td
4568

read only -> read-only

4569

read only -> read-only

clang/lib/Sema/SemaDeclAttr.cpp
6633

I think this should use err_attribute_argument_type instead, since there's only one argument.

clang/test/CodeGen/guard_nocf.c
54

Please add a newline to the end of the file.

clang/test/CodeGenCXX/guard_nocf.cpp
85

Please add a newline to the end of the file.

ajpaverd updated this revision to Diff 236572.Jan 7 2020, 6:14 AM
ajpaverd marked 5 inline comments as done.

Make guard_nocf an attribute of individual call instructions

Instead of using "guard_nocf" as a function attribute, make this an attribute on individual call instructions for which Control Flow Guard checks should not be added. This allows the attribute (or lack thereof) to be propagated with the call instructions if they are inlined into another function.

Can you please add some Sema tests that verify the attribute only appertains to functions, accepts the right number of arguments, etc? Also, some tests showing how this attribute works for things like redelcarations and virtual functions would help.

Thanks for the feedback @aaron.ballman! I've added tests that should match the behaviour of MSVC for each of the cases you suggested (as well as function inlining). Any other test cases we should add?

Thanks for doing this!

When I applied this patch and added __declspec(guard(nocf)) to the function that was giving us trouble, I still crashed with a CFG failure in the caller, since the nocf function was inlined. When I additionally marked that function as noinline, then the CFG checks were omitted, as desired. Is this behavior with respect to inlining expected?

You may want to mention "PR44096" in the commit message.

Thanks for testing this @dmajor! I think the expected behaviour is that the modifier (or lack thereof) should be preserved even if the function is inlined. The latest changes should achieve this by placing the "guard_nocf" attribute on the individual indirect call instructions rather than the function itself. Does this now work for your function without having to add noinline?

dmajor added a comment.Jan 7 2020, 3:24 PM

Is the current patch an interdiff? It would be helpful to have the diff against the master repo; Phabricator can take care of showing interdiffs if necessary.

ajpaverd updated this revision to Diff 236806.Jan 8 2020, 6:13 AM
  • Make guard_nocf an attribute of individual call instructions
aaron.ballman added inline comments.Jan 8 2020, 12:09 PM
clang/include/clang/Basic/Attr.td
2914

Should we also support ObjC methods? What about things like lambdas or blocks?

(Note, these could all be handled in a follow-up patch, I just wasn't certain if this attribute was specific to functions or anything that executes code.)

clang/lib/CodeGen/CGCall.cpp
4421

const auto * because the type is spelled out in the initialization anyway.

4422–4427
if (const auto *A = FD->getAttr<CFGuardAttr>()) {
  if (A->getGuard() == CFGuardAttr::GuardArg::nocf && !CI->getCalledFunction())
    Attrs = ...
}

(This way you don't have to call hasAttr<> followed by getAttr<>.)

clang/test/Sema/attr-guard_nocf.cpp
1 ↗(On Diff #236806)

Since this test file is identical to the C test, I would remove the .cpp file and add its RUN line to the .c test instead. You can do this with:

// RUN: %clang_cc1 -triple %ms_abi_triple -fms-extensions -verify -std=c++11 -fsyntax-only -x c++ %s
dmajor added a comment.Jan 8 2020, 1:25 PM

I've confirmed that the current patch fixes our CFG failures. Thanks again!

ajpaverd updated this revision to Diff 236906.Jan 8 2020, 2:31 PM
ajpaverd marked 7 inline comments as done.
  • Address comments

Thanks for the feedback @aaron.ballman and @dmajor!

clang/include/clang/Basic/Attr.td
2914

Good point! At the moment this attribute is only documented for functions, so I'll have to investigate the expected behaviour for lambdas, blocks, and ObjC methods and provide a follow-up patch.

clang/lib/CodeGen/CGCall.cpp
4422–4427

That's much nicer - thanks!

clang/test/Sema/attr-guard_nocf.cpp
1 ↗(On Diff #236806)

That saves a lot of repetition - thanks!

aaron.ballman accepted this revision.Jan 9 2020, 6:29 AM

LGTM!

clang/include/clang/Basic/Attr.td
2914

That sounds good to me, thanks!

This revision is now accepted and ready to land.Jan 9 2020, 6:29 AM
This revision was automatically updated to reflect the committed changes.