Page MenuHomePhabricator

LLD: CET shadow stack support on Windows
AcceptedPublic

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

Details

Reviewers
ruiu
jhenderson
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.Fri, Nov 22, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Nov 22, 10:41 AM

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

penzn updated this revision to Diff 230720.Fri, Nov 22, 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.Fri, Nov 22, 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.Fri, Nov 22, 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.Sun, Nov 24, 4:58 PM
llvm/test/tools/llvm-readobj/coff-cet-compat.test
11–15

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.Mon, Nov 25, 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
11–15

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.Mon, Nov 25, 6:34 PM
llvm/test/tools/llvm-readobj/coff-cet-compat.test
11–15

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

mstorsjo added inline comments.Mon, Nov 25, 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.Tue, Nov 26, 12:42 PM

Remove unused MinGW flag

penzn marked an inline comment as done.Tue, Nov 26, 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
11–15

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.Tue, Nov 26, 8:21 PM

LGTM with these fixes.

lld/COFF/Writer.cpp
172

The constructor should take uint32 instead of unsigned.

173

C -> c

175–177

Let's just return 4.

180

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

180

B -> buf

183

Characteristics -> characteristics

957

Break this line so that it fits within 80 columns.

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

80 cols

753

80 cols

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

Incorporate review feedback

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

Thank you!

lld/COFF/Writer.cpp
175–177

Just curious, why this way - easier to read?

ruiu added inline comments.Wed, Nov 27, 7:34 PM
lld/COFF/Writer.cpp
175–177

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.Tue, Dec 3, 1:53 PM

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

ruiu added a comment.Wed, Dec 11, 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.EditedWed, Dec 11, 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.Wed, Dec 11, 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!