Page MenuHomePhabricator

LLD: CET shadow stack support on Windows
ClosedPublic

Authored by penzn on Nov 22 2019, 10:41 AM.

Details

Summary

This patch implements CET Shadow Stack (Intel Controlflow Enforcement Technology) support on Windows.

Windows support for CET is limited to shadow stack, which is enabled by setting a PE bit in the linker.

Docs:

Diff Detail

Event Timeline

penzn created this revision.Nov 22 2019, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 22 2019, 10:41 AM

Can you reupload the patch with full context. Using -U999999 on your diff command

penzn updated this revision to Diff 230720.Nov 22 2019, 2:04 PM

Full(er) patch for the same change.

mstorsjo added inline comments.
lld/MinGW/Options.td
105 ↗(On Diff #230720)

FWIW, if you want the option to actually have any effect when passed to the mingw driver, you need a couple lines in MinGW/Driver.cpp (and a corresponding test in test/MinGW/driver.test).

penzn added inline comments.Nov 22 2019, 2:36 PM
lld/MinGW/Options.td
105 ↗(On Diff #230720)

Good point, thank you. That is intentional - I noticed that other MSVC security flags have no effect in MinGW driver.

There is a patch for GNU-compatible linker on Linux, D59780, though I am not sure if we can simply wire those options to COFF implementation introduced here. The goal of this patch is to support the VS-compatible flag.

I might be wrong with my assumptions on MinGW driver, any clarification would be greatly appreciated.

mstorsjo added inline comments.Nov 22 2019, 2:54 PM
lld/MinGW/Options.td
105 ↗(On Diff #230720)

Well, some of them, like nxcompat, are enabled by default within lld, and I'm not sure there's any GNU linker option to disable it, so therefore we don't actually act upon that option, while other flags like dynamicbase do exist and are forwarded.

As there's no predecent for these options being used in the wild with GNU ld for mingw so far, you could just omit it from this file if you aren't planning on hooking it up, as it won't be necessary for compatibility with existing projects built for mingw.

But I presume it could make sense to enable this flag in mingw configurations as well (would it?), and in that case, hooking it up with the same option name as for the ELF frontend, forwarding it to the link/COFF style flag name would be most welcome.

ruiu added inline comments.Nov 24 2019, 4:58 PM
llvm/test/tools/llvm-readobj/coff-cet-compat.test
10–14 ↗(On Diff #230720)

It looks like there are still a few unused bit in the real DLLCharacteristics bit (https://docs.microsoft.com/en-us/windows/win32/debug/pe-format#dll-characteristics), so it is a bit odd that Microsoft chose to define an "extended" DLLCharacteristics field now, but well, if they chose this format we'll have to follow.

Are AddressOfRawData and PointerToRawData significant for ExtendedCharacteristics entries? I'd b tempted to fill them with zero, as they don't seem to make any meaning in this context.

penzn added inline comments.Nov 25 2019, 6:10 PM
lld/MinGW/Options.td
105 ↗(On Diff #230720)

GNU would have its own CET flags, there is an effort to port them to LLD in D59780. I can remove the flag from MinGW to avoid confusion, though I don't feel strongly either way. What do you think?

llvm/test/tools/llvm-readobj/coff-cet-compat.test
10–14 ↗(On Diff #230720)

Yep, I am also not sure why this was necessary either, may be they are seeing some important use for the new field (right now it a can only contain this bit).

AddressOfRawData and PointerToRawData are coming from MSVC linker, I think that is where the bit is stored. I don't know if it is possible to "inline" it into the entry somehow:

If the Type field is set to IMAGE_DEBUG_TYPE_EX_DLLCHARACTERISTICS, the debug raw data contains extended DLL characteristics bits. [...]

ruiu added inline comments.Nov 25 2019, 6:34 PM
llvm/test/tools/llvm-readobj/coff-cet-compat.test
10–14 ↗(On Diff #230720)

I mean my question is what values does MSVC link.exe sets to AddressOfRawData and PointerToRawData?

mstorsjo added inline comments.Nov 25 2019, 11:01 PM
lld/MinGW/Options.td
105 ↗(On Diff #230720)

Yes, I'd prefer to not add any code for the MinGW driver until it does something meaningful in this case.

penzn updated this revision to Diff 231122.Nov 26 2019, 12:42 PM

Remove unused MinGW flag

penzn marked an inline comment as done.Nov 26 2019, 1:42 PM
penzn added inline comments.
lld/MinGW/Options.td
105 ↗(On Diff #230720)

Done. We can revisit this in a separate patch - and tie Windows CET functionality to shadowstack option in GNU-style flag.

llvm/test/tools/llvm-readobj/coff-cet-compat.test
10–14 ↗(On Diff #230720)

I think it allocates a 4-byte value, sets CET bit in it, and points both AddressOfRawData and PointerToRawData to it. At least that what it looks like from the outside, though I can be wrong.

ruiu accepted this revision.Nov 26 2019, 8:21 PM

LGTM with these fixes.

lld/COFF/Writer.cpp
171

The constructor should take uint32 instead of unsigned.

172

C -> c

174–176

Let's just return 4.

179

Likewise, I'd replace sizeof() with 4.

179

B -> buf

182

Characteristics -> characteristics

951

Break this line so that it fits within 80 columns.

llvm/tools/llvm-readobj/COFFDumper.cpp
754

80 cols

756

80 cols

This revision is now accepted and ready to land.Nov 26 2019, 8:21 PM
penzn updated this revision to Diff 231332.Nov 27 2019, 4:19 PM

Incorporate review feedback

penzn marked 9 inline comments as done.Nov 27 2019, 4:29 PM

Thank you!

lld/COFF/Writer.cpp
174–176

Just curious, why this way - easier to read?

ruiu added inline comments.Nov 27 2019, 7:34 PM
lld/COFF/Writer.cpp
174–176

Yeah. If we return 4 the size of this record is obviously 4 bytes. Since this class is so small, I don't think that we need any abstraction to hide the magic number.

penzn marked an inline comment as done.Dec 3 2019, 1:53 PM

Can you help me merge this? I don't have commit access.

ruiu added a comment.Dec 11 2019, 12:27 AM

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

On this topic, yaml2obj has COFF support, although I don't know if that's sufficient for your use case here.

penzn added a comment.EditedDec 11 2019, 10:02 AM

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

It is a dummy executable (along the lines of int main() {return 0;}) built with Visual Studio 2019. This was inspired by the existing llvm-readobj PDB test which checks has_pdb.exe.

On this topic, yaml2obj has COFF support, although I don't know if that's sufficient for your use case here.

yaml2obj does not yet have CET flag support. What is the preferred path in this situation? We can add it, but it would be only used in this test.

ruiu added a comment.Dec 11 2019, 7:12 PM

Could you tell me about how you generated has_cet.exe? I wonder if there's a way to create an executable in the test instead of submitting a binary file to the repository.

It is a dummy executable (along the lines of int main() {return 0;}) built with Visual Studio 2019. This was inspired by the existing llvm-readobj PDB test which checks has_pdb.exe.

On this topic, yaml2obj has COFF support, although I don't know if that's sufficient for your use case here.

yaml2obj does not yet have CET flag support. What is the preferred path in this situation? We can add it, but it would be only used in this test.

I'd prefer enhancing yaml2obj tool so that it can emit the extended characteristics field if it is not too hard.

On this topic, yaml2obj has COFF support, although I don't know if that's sufficient for your use case here.

yaml2obj does not yet have CET flag support. What is the preferred path in this situation? We can add it, but it would be only used in this test.

I'd prefer enhancing yaml2obj tool so that it can emit the extended characteristics field if it is not too hard.

In general, I'm always in favour of enhancing the tool where possible. Binary files are always opaque and hard to understand what is in them, so yaml2obj should be extended if it's sensible to do so. The more we do that, the less easier future tests will be to write!

penzn added a comment.Jan 3 2020, 4:42 PM

yaml2obj is capable of producing binary .rdata section (which would host CET bit) from hex, but not individual debug directories that would go into it. Looks like all the tests dealing wit debug directories are using pre-built binaries - codeview tests are a good example).

penzn added a comment.Jan 29 2020, 1:57 PM

@ruiu can we separate enhancing yaml2obj from this change? I would be more than happy to help with implementing debug directory support as a separate change, to avoid making this change much more involved.

penzn edited the summary of this revision. (Show Details)Jan 29 2020, 2:02 PM
penzn added a comment.Feb 12 2020, 2:20 PM

Expanding on my previous comment. In its current state yaml2obj is not aware of the section contents, and I think adding this functionality would be a bit more involved than the code we have in this review so far.

Quick rundown of what would be needed for yaml2obj change:

  • Define YAML syntax for debug entries
  • Support debug directory and its contents inside section mapping in ObjectYAML
    • Need to represent the structure from Debug section down to individual debug records
  • Change other tests which depend on debug directory structure

We should probably discuss details. For example, can debug directory support in ObjectYAML take inspiration from LLD or would that make testing moot? Would ObjectYAML need any tests for this new functionality?

Do you mind if we move ObjectYAML discussion elsewhere? What would be a good place for it, llvm-dev?

As for this change, another option is to use yaml2obj, but output debug directory as a hex section in the YAML file. However this would inconsistent with codeview tests, maybe it would be better to change everything at once when yaml2obj is ready.

Expanding on my previous comment. In its current state yaml2obj is not aware of the section contents, and I think adding this functionality would be a bit more involved than the code we have in this review so far.

Quick rundown of what would be needed for yaml2obj change:

  • Define YAML syntax for debug entries
  • Support debug directory and its contents inside section mapping in ObjectYAML
    • Need to represent the structure from Debug section down to individual debug records
  • Change other tests which depend on debug directory structure

    We should probably discuss details. For example, can debug directory support in ObjectYAML take inspiration from LLD or would that make testing moot? Would ObjectYAML need any tests for this new functionality?

    Do you mind if we move ObjectYAML discussion elsewhere? What would be a good place for it, llvm-dev?

    As for this change, another option is to use yaml2obj, but output debug directory as a hex section in the YAML file. However this would inconsistent with codeview tests, maybe it would be better to change everything at once when yaml2obj is ready.

I'm not sure I really know enough about the intricacies here to provide good feedback. I do think splitting out any yaml2obj functionality into a separate change makes sense though. When it comes to using a hex section/adding yaml2obj support for that section type, my opinion is that if the section only needs to be a few bytes long, it's fine, but if it's more than that and the hex contents are essentially unreadable, then there's no benefit in using yaml2obj at all. If you want to continue the discussion separately, llvm-dev is probably the best place for it.

penzn added a comment.Tue, Mar 3, 7:17 PM

I've started the discussion on the mailing list: http://lists.llvm.org/pipermail/llvm-dev/2020-March/139706.html

@ruiu should we merge this change in the meantime? When yaml2obj change would happen it would be pretty straightforward to change the test introduced here.

ruiu accepted this revision.Tue, Mar 3, 10:18 PM

LGTM with this change.

As to how to test this, I'm fine with checking in a binary file until you extend yaml2obj so that the tool can produce an ExtendedDLLCharacterstics section. It isn't ideal but we already have a few binary files in our test suite.

lld/COFF/Writer.cpp
179

You should use write32le to guarantee that you are writing the four bytes in the little-endian order. Windows machines are all little-endian, but if you by any chance cross-build a Windows binary on a big-endian machine (e.g. creating an UEFI or Windows binary on PPC32BE using lld), this code would write a word in a wrong order. I believe there's a PPC big-endian bot, so this code would fail on the bot if you check this in as-is.

penzn updated this revision to Diff 248389.Wed, Mar 4, 9:16 PM

Use write32le to set extended DLL characteristics.

penzn marked an inline comment as done.Wed, Mar 4, 9:21 PM

Another minor edit, I moved COFF test files for llvm-readobj to the directory where the rest of them are now.

LGTM with this change.

As to how to test this, I'm fine with checking in a binary file until you extend yaml2obj so that the tool can produce an ExtendedDLLCharacterstics section. It isn't ideal but we already have a few binary files in our test suite.

Thank you! Made the change. I don't have commit rights, help with the merge will be greatly appreciated.

Looks like there's a lot of clang-format issues in this that need addressing before this gets committed?

ruiu added a comment.Thu, Mar 5, 9:48 PM

I tried to apply your patch to submit, but it looks like your test file (llvm/test/tools/llvm-readobj/COFF/Inputs/has_cet.exe) cannot be downloaded from Phabricator. Can you upload a file somewhere and send me the URL? Or you can encode with base64 and send it to me via email.

penzn updated this revision to Diff 250117.EditedThu, Mar 12, 6:43 PM

Formatting; also instructions on building test image.

base64 encoding attached, also made a gist for it: https://gist.github.com/penzn/0c512ba24f615b91c2bccbcc38bfd933

I am trying to attach unencoded file here as well, let's see if Phabricator would agree.

MaskRay added inline comments.Thu, Mar 12, 9:31 PM
llvm/test/tools/llvm-readobj/COFF/cetcompat.test
5

We should avoid a checked-in binary if possible. Can't it be synthesized from a YAML file?

ruiu added a comment.Fri, Mar 13, 3:45 AM

That's discussed on llvm-dev, and I think that even though we should ideally extend
the tool to handle the new bit, it shouldn't block this patch. This patch adds a missing
flag that was added to MSVC link.exe recently.

penzn marked 2 inline comments as done.Fri, Mar 13, 1:40 PM

I had to edit the comment above, looks like I took snapshot from directory when I was reproducing build instructions, instead of actual test directory. Now the exe and base64 encoding above (in the gist as well) are correct and the test should pass.

llvm/test/tools/llvm-readobj/COFF/cetcompat.test
5

Not at this stage, yaml2obj still needs work to support it. As @ruiu pointed out there is discussion on llvm-dev about making that change.

This revision was automatically updated to reflect the committed changes.