This is an archive of the discontinued LLVM Phabricator instance.

[CFGuard] Add address-taken IAT tables and delay-load support
ClosedPublic

Authored by ajpaverd on Sep 11 2020, 12:41 PM.

Details

Summary

This patch adds support for creating Guard Address-Taken IAT Entry Tables (.giats$y sections) in object files, matching the behavior of MSVC. These contain lists of address-taken imported functions, which are used by the linker to create the final GIATS table.
Additionally, if any DLLs are delay-loaded, the linker must look through the .giats tables and add the respective load thunks of address-taken imports to the GFIDS table, as these are also valid call targets.

Diff Detail

Event Timeline

ajpaverd created this revision.Sep 11 2020, 12:41 PM
ajpaverd requested review of this revision.Sep 11 2020, 12:41 PM
rnk added a reviewer: hans.Sep 16 2020, 12:54 PM
rnk added inline comments.Sep 16 2020, 1:57 PM
lld/COFF/Writer.cpp
1683

Can we set either ProtectDelayLoadIAT or DelayLoadIATSection now, or does that take more work?

1762

These two methods are essentially the same up until here. Your version seems more general. Can you rewrite markSymbolsForRVATable in terms of this utility? This code shouldn't be hot, I don't think we need to worry about the cost of an extra temporary vector of address-taken symbols.

dmajor added a subscriber: dmajor.Sep 22 2020, 1:15 PM
ajpaverd updated this revision to Diff 294768.Sep 28 2020, 11:15 AM
ajpaverd marked 2 inline comments as done.

Rewrite markSymbolsForRVATable() in terms of getSymbolsFromSections()

lld/COFF/Writer.cpp
1683

I think this will take more work (e.g. the latter requires moving the Delayload import table into its own .didat section).

rnk accepted this revision.Sep 28 2020, 11:23 AM

lgtm

This revision is now accepted and ready to land.Sep 28 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.

I believe this is causing https://crbug.com/1134215:

ninja -t msvc -e environment.x86 -- ..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /nologo -libpath:..\..\third_party\llvm-build\Release+Asserts\lib\clang\12.0.0\lib\windows -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\lib\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\um\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\ucrt\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\atlmfc\lib\x86 /IMPLIB:./browser_switcher_bho.dll.lib /DLL /OUT:./browser_switcher_bho.dll /PDB:./browser_switcher_bho.dll.pdb @./browser_switcher_bho.dll.rsp
unknown symbol kind
UNREACHABLE executed at C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Symbols.h:443!
PLEASE submit a bug report to https://crbug.com and run tools/clang/scripts/process_crashreports.py (only works inside Google) which will upload a report and include the crash backtrace.
Stack dump:
0.	Program arguments: ..\..\third_party\llvm-build\Release+Asserts\bin\lld-link.exe /nologo -libpath:..\..\third_party\llvm-build\Release+Asserts\lib\clang\12.0.0\lib\windows -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\lib\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\um\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\win_sdk\Lib\10.0.19041.0\ucrt\x86 -libpath:..\..\third_party\depot_tools\win_toolchain\vs_files\a687d8e2e4114d9015eb550e1b156af21381faac\VC\Tools\MSVC\14.26.28801\atlmfc\lib\x86 /IMPLIB:./browser_switcher_bho.dll.lib /DLL /OUT:./browser_switcher_bho.dll /PDB:./browser_switcher_bho.dll.pdb @./browser_switcher_bho.dll.rsp 
 #0 0x00007ff7471970b5 HandleAbort C:\b\s\w\ir\cache\builder\src\third_party\llvm\llvm\lib\Support\Windows\Signals.inc:408:0
 #1 0x00007ff7495b0063 raise minkernel\crts\ucrt\src\appcrt\misc\signal.cpp:547:0
 #2 0x00007ff7495aa9d0 abort minkernel\crts\ucrt\src\appcrt\startup\abort.cpp:71:0
 #3 0x00007ff74716d416 llvm::llvm_unreachable_internal(char const *, char const *, unsigned int) C:\b\s\w\ir\cache\builder\src\third_party\llvm\llvm\lib\Support\ErrorHandling.cpp:213:0
 #4 0x00007ff7471f067c lld::coff::Defined::getChunk(void) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Symbols.h:443:0
 #5 0x00007ff747244292 addSymbolToRVASet C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:1542:0
 #6 0x00007ff747245187 `anonymous namespace'::Writer::createGuardCFTables C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:1655:0
 #7 0x00007ff747246283 `anonymous namespace'::Writer::createMiscChunks C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:988:0
 #8 0x00007ff74724bbb8 `anonymous namespace'::Writer::run C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:614:0
 #9 0x00007ff74724d439 lld::coff::writeResult(void) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Writer.cpp:311:0
#10 0x00007ff7471f852a lld::coff::LinkerDriver::link(class llvm::ArrayRef<char const *>) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Driver.cpp:2143:0
#11 0x00007ff7471f8af0 lld::coff::link(class llvm::ArrayRef<char const *>, bool, class llvm::raw_ostream &, class llvm::raw_ostream &) C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\COFF\Driver.cpp:96:0
#12 0x00007ff747158898 lldMain C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\tools\lld\lld.cpp:155:0
#13 0x00007ff747158ce7 main C:\b\s\w\ir\cache\builder\src\third_party\llvm\lld\tools\lld\lld.cpp:211:0
#14 0x00007ff749583a04 __scrt_common_main_seh d:\A01\_work\6\s\src\vctools\crt\vcstartup\src\startup\exe_common.inl:288:0
#15 0x00007ff9c0982774 (C:\Windows\System32\KERNEL32.DLL+0x12774)
#16 0x00007ff9c0a90d51 (C:\Windows\SYSTEM32\ntdll.dll+0x70d51)

Printing out the kind() of the failing symbol, it gives 40, which is nowhere in the range of Symbol::Kind. Reverting for now, let me know if you need more details for fixing this.

We don't normally revert commits for downstream build failures. Please can you submit a test case that triggers this issue?

Working on a test case right now.

turns out it was a variable not being initialized: https://reviews.llvm.org/D88744

turns out it was a variable not being initialized: https://reviews.llvm.org/D88744

Whoops, forgot that was on top of this patch. Commented inline in this patch

lld/COFF/Symbols.h
352

this needs to be initialized

ajpaverd marked an inline comment as done.Oct 5 2020, 10:53 AM

Thanks for the diagnostics @aeubanks! Does this now resolve the issue with the Chrome build?

When I was looking into it, after initializing loadThunkSym to nullptr, the build step that was failing then succeeded. So that should be good enough.

ajpaverd updated this revision to Diff 296233.EditedOct 5 2020, 11:03 AM
ajpaverd added a reviewer: aeubanks.

Always initialize loadThunkSym to nullptr.

lgtm, feel free to reland

Thanks @aeubanks! I don't yet have commit access and my colleague is on leave.

This caused a regression when calling through function pointers in a certain way as seen in sqlite3: https://bugs.llvm.org/show_bug.cgi?id=47905

hans added a comment.Nov 11 2020, 7:08 AM

This caused a regression when calling through function pointers in a certain way as seen in sqlite3: https://bugs.llvm.org/show_bug.cgi?id=47905

Reverted in 418f18c6cdfe56e77669e2f4d3df3bca1020156d due to that.

ajpaverd reopened this revision.Nov 13 2020, 2:07 PM
This revision is now accepted and ready to land.Nov 13 2020, 2:07 PM
ajpaverd updated this revision to Diff 305260.EditedNov 13 2020, 2:07 PM

Correctly handle __imp_ symbols in .giats and .gfids sections

For address-taken dllimport functions with a corresponding __imp_ symbol defined, add the __imp_ symbol to the .giats section. Always add the symbols of address-taken functions to the .gfids section.

ajpaverd requested review of this revision.Nov 13 2020, 2:15 PM
ajpaverd added a reviewer: dmajor. ajpaverd added 1 blocking reviewer(s): hans.

@hans and @dmajor thanks for reporting this issue (https://bugs.llvm.org/show_bug.cgi?id=47905).
Do the latest changes resolve this in Chromium and sqlite3?

hans added a comment.Nov 16 2020, 6:24 AM

For Chromium, the latest patch seems to fix the issues we saw. Thanks!

Firefox tests look good as well.

would you like me to reland this?

This revision was not accepted when it landed; it landed in state Needs Review.Nov 17 2020, 6:32 PM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 9 2020, 7:52 AM

FYI: While the reland didn't break anything, it apparently also didn't fix everything it set out to fix, see https://bugs.chromium.org/p/chromium/issues/detail?id=1132179#c13 and onwards.