Repository navigation
fix(relinker): refuse GS traps that overwrite a code entry point in Windows output - #11090
Conversation
…indows output --windows replaced a guest GS memory instruction with UD2 and NOPs even when a symbol, a relocation pointer, a lea target or a DT_INIT_ARRAY entry started inside it, so that entry ran padding instead of the original code. A trapped instruction that holds a collector entry root after its first byte now fails with "GS trap would overwrite an executable entry point" at the root's file offset, the rule that TLS rewrites already follow. The branch check runs first, so a relative branch into the instruction keeps its message, and outputs without such a root are unchanged.
|
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
I ran |
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed exact head 33b48ba39f0155d466ae028e6d838a8611e83bb1. The Windows GS trap now checks the next executable entry root strictly inside the instruction span before replacing the bytes, while preserving the earlier branch-target error path. The fixture covers relocation, symbol, LEA and initializer roots, plus an entry at the instruction start and a safe trap. I ran gs_trap_entry_roots and windows_tls_protected_entries locally (2/2 pass), git diff --check upstream/main...HEAD, and tools/check_conventions.py --base upstream/main --head HEAD (pass). Hosted Linux, Windows, macOS/arm64 and ASan relinker checks pass. Hosted macOS/x86_64 reports one failure in unrelated masked_store_lowering (“Masked store stub changed the flags”); the PR’s new and adjacent tests passed there. I have not run a macOS build locally.
|
@MarcZX10 Thanks. |
klosixgaming-arch
left a comment
There was a problem hiding this comment.
Reviewed head 33b48ba39f by reading the diff and the existing review thread. I did not build or run this, so everything below is from the code, not from a test of mine.
What it does. The Windows TLS builder used to replace a guest GS-prefixed memory instruction with a trap (ud2 plus padding) without checking whether an executable entry point lay inside the bytes it overwrote. Now, if an entry root falls strictly inside the instruction (after its first byte), conversion fails with "GS trap would overwrite an executable entry point" and the offset; a root exactly at the start of the instruction is still fine.
Checked: entryRoots.upper_bound(*instruction) gives the first root after the instruction start, and *entry - *instruction < info.Length limits it to inside the instruction, so a root at the start is excluded by upper_bound and one at start + Length is excluded by the comparison. The file offset is computed from the section header the same way as the neighbouring code. The new gs_trap_entry_roots test covers four ways an entry root can land there (relative, symbol, LEA, initializer) and two cases that must still convert (a safe image and an entry root at the instruction start), and it checks the trapped bytes.
Notes:
- This turns a silent corruption into a hard conversion failure. That is the right call, but it also means an image that used to convert (and then misbehave) will now be refused with no workaround. A debt entry or a line in the relinker docs saying what to do when you hit it would help the next user.
- The hosted macOS x86_64 job reported one failure,
masked_store_lowering. The author says the PR does not touch it, and the diff agrees (onlyWindowsTlsBuilder.cpp, a new test and its registration). MarcZX10 ran the new test locally from the head. I'd still re-run that job once to confirm it was a flake, because a "pre-existing failure" is easy to claim and cheap to check.
Approving.
AI-assisted: yes. Written with AI assistance and reviewed by me.
…s-gs-trap-entry-roots
|
@klosixgaming-arch Merged current |
A Windows conversion now fails when a code entry lies after the first byte of a guest GS instruction that the TLS builder replaces with UD2. The new debt entry names both messages, says that no option skips the checks and that Linux output keeps the instruction, so a user who hits the refusal knows which target to use.
|
@klosixgaming-arch Thanks for the review. Merged current |
What
--windowsreplaces a guest GS memory instruction with UD2 and NOPs and refuses only a relative branch into it (#10710). On untouched 4017310,65 48 8b 04 25 28 00 00 00 c3at 0x210 with an entry root at 0x214 (aSTT_FUNCsymbol, anR_X86_64_RELATIVEpointer, a REX.Wleatarget or aDT_INIT_ARRAYentry) converted with exit 0, and the PE held0f 0b 90 90 90 90 90 90 90 c3at RVA 0x10210, so the entry at 0x214 ran NOPs andretinstead ofand eax,0x28; ret, which Linux output keeps.CodeInstructionCollectordoes not refuse this layout: its overlap rule fires only when the overlapped span holds a0x64byte or the previous instruction has an FS prefix, and the same layout with64already fails withCode analysis: overlapping instruction boundaries.WindowsTlsBuilder::Buildrecords for each trapped GS memory instruction[start, start + length)the first collector entry root withstart < root < start + length, found with the sameentryRoots.upper_boundlookup that TLS rewrites use (fix(relinker): protect Windows TLS code entry roots #5890). In the trap loop, after the existingBranch enters a guest GS instructioncheck, such a root throwsGS trap would overwrite an executable entry pointat the root's file offset; the CLI exits 2 and writes no output.entryRootsalso contains relative branch targets, so the branch check stays first and keeps its message and offset. A root at the instruction start is still trapped as before.entryRootsalso holds pointer targets the collector takes as data, so such a target inside a trapped instruction is refused too, as TLS rewrites already do.checkProtectedEntryis not reused because its message names a TLS rewrite. Windows only; the macOS counterpart inMacOsImage.cppis a follow-up PR, becauseScanTlscollects withoutentryRootson the line fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 rewrites. A new entry indocs/dev/debt/relinker.md, before the null-symbol relocation entry, states both refusals, that the messages give the position of the entered byte, that no option skips the checks, and that Linux output keeps the instruction with libkernel'ssysarchsetting the GS base; the Windows GS entry and the macOS GS entry are unchanged.test_gs_trap_entry_roots.py, built onmake_imageandpe_bytes_atfromtest_windows_tls_protected_entries.py, with the GS load at 0x210: rootsrelative,symbol,leaandinitializermust fail with the message,(offset 0x214)and no output; controlssafeandaccess-startmust convert with0f 0b 90 90 90 90 90 90 90 c3at RVA 0x10210. Registered incore/relinker/CMakeLists.txtafterwindows_tls_protected_entries.mainf95acfc: 32 other open pull requests touchcore/relinker/CMakeLists.txtorWindowsTlsBuilder.cpp. All of them except feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021, fix(libc): use the default heap for an executable without process parameters #6713 and fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 merge withmainand with this branch without conflicts. Those three conflict withmain, none of them in lines of this PR. feat(aot): consolidate native ARM64 compiler, runtime and consumers #5021 addsmacos_fixturesaftermacos_output, more than 100 lines belowgs_trap_entry_roots(line 554, afterwindows_tls_protected_entries). fix(libc): use the default heap for an executable without process parameters #6713 addswindows_process_parametersafterelf_headerand conflicts withmainonly indocs/dev/debt/libc.md. fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 changescore/relinker/CMakeLists.txtat its lines 248 and 774. InWindowsTlsBuilder.cppit changes the signature ofBuildand theCollectcall three lines below thegsTrapsdeclaration that this PR changes. It conflicts withmaininGuestModuleBuilder.cpp,RelinkerPipeline.cppanddocs/dev/debt/relinker.md. ItsextraEntriesbecome collector entry roots, so after both merge, a GS trap that holds one after its first byte is refused as well.docs/dev/debt/relinker.mdis touched by 27 open pull requests. With the entry inserted before the null-symbol entry,git merge-tree --write-treeagainst each of them from its merge base with this branch reports the same conflicting files as without the entry, and the existing conflict hunk of fix(relinker): analyze referenced code beyond SCE scan boundaries #10861 in that file is unchanged; an extension of the Windows GS entry in place would have conflicted with fix(relinker): reject an executable PT_LOAD alignment that is not a power of two #10518, which inserts an entry directly before it.Tested
Windows 11, GCC 15.2.0 (WinLibs UCRT), relinker-only Release build (
-DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON); synthetic fixtures only, no title or homebrew, relinked outputs not run.gs_trap_entry_rootson untouched 4017310 fails withAssertionError: ('relative', 0, ''); run case by case, all four root inputs exited 0 with an output file and0f 0b 90 90 90 90 90 90 90 c3at RVA 0x10210, and both controls passed. With the change all four exit 2 withFAIL: GS trap would overwrite an executable entry point (offset 0x214)and no output, the controls are unchanged, and the test passes in 0.4 s.ctest -R 'gs_trap|tls_function_coverage|windows_tls|windows_gs' -j 4: 38 tests passed in 26.4 s, among themgs_trap_entry_roots(0.8 s),windows_tls_protected_entries,windows_tls_zero_fill,windows_tls_thread_pointer,tls_function_coverage_gs_trapsandtls_function_coverage_gs_traps_implicit.macos_gs_trapsalso passed.main(f95acfc) into the branch without conflicts (66f6fa3); since 3a2c51a,mainchangedWindowsTlsBuilder.cpponly through fix(relinker): accept empty TLS images beyond EOF on Windows #11000 (an empty-TLS check after the trap loop) andcore/relinker/CMakeLists.txtaway from this PR's registration. On 66f6fa3,ctest -R 'gs_trap|tls_function_coverage|windows_tls|windows_gs' -j 6: 39 tests passed, among themgs_trap_entry_roots(1.3 s),windows_tls_protected_entries,windows_tls_zero_fill,windows_tls_thread_pointer,macos_gs_traps,tls_function_coverage_gs_trapsandtls_function_coverage_gs_traps_implicit.ctest --test-dir build-relinker -j 6: 311 tests, 308 passed,long_pathsskipped; failures known on this machine onmainas well:select_tests(Windows symlink fixture) andpatch_advisor_unit(path with spaces, which open test(tools): compare quoted and resolved paths in the patch advisor test #10667 fixes); both failed again alone.docs/dev/debt/relinker.md; the test runs above are on 66f6fa3, whose code and tests are the same.python tools/check_conventions.py --base upstream/mainwith this title and description passed on 823251d.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)