Repository navigation
fix(relinker): reject Linux relocations into non-writable PT_LOAD segments - #13977
roboterspinne wants to merge 2 commits into
Conversation
…ments A Linux relink accepted a RELA or JMPREL entry whose r_offset lies in a PT_LOAD without PF_W and wrote an output that keeps the segment read-only without DT_TEXTREL, so ld.so writes into a read-only mapping. The check over relocation offsets now requires a writable PT_LOAD, as the gABI does for objects without DT_TEXTREL; a PT_LOAD without flags still counts as writable, and an R_X86_64_NONE entry, which writes nothing, only has to lie inside a PT_LOAD as before. Test fixtures that placed GOT slots or relocation targets in their read-execute PT_LOAD now give them a writable one.
…able R_X86_64_NONE exception With --lazy-binding ld.so stores its link map and resolver in the first slots at DT_PLTGOT, so that range now has to lie in a writable PT_LOAD like every relocation target. The pipeline never passes R_X86_64_NONE entries to the Linux patcher, so the patcher needs no exception for them.
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
At head 50b37bb1a98edde1d306775c870a20440fd26814 on base 73f4f7e06e4f210a8c6570e335fd654dc0a86134, the new writable-segment check still accepts a PT_LOAD with p_flags == 0 as writable.
What I ran
Linux x86_64, Python 3.11.2, GCC 12.2.0, Git 2.39.5. The dependency/merge preview and source checks were run; I did not build or run C++ tests locally. Relinker CI was still running when checked.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13977
gh pr checks 13977 -R boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff check | Passed |
| Conventions and PR template | Passed |
| Dependency and merge preview | Passed |
| Hosted conventions, intake, eligibility, labels | Passed |
| Hosted relinker platform jobs and ASAN | In progress/queued |
| Local C++ tests | Not run |
Precheck output
All conventions checks passed.
Negative control
| Assertion | Change to code under test | Did the new test fail? |
|---|---|---|
A relocation into a segment without PF_W is rejected |
The new relative-in-flagless case sets p_flags to zero and expects success |
Not run locally; source inspection shows the case is accepted by the current guard. |
Findings
Blocking
-
RequireLoadedstill acceptsheader.Flags == 0, and the newrelative-in-flaglesscase expects that relocation to pass. In Linux, the ELF loader derivesPROT_WRITEfromPF_W; a zerop_flagsvalue therefore does not describe a writable segment. The relocation is accepted by this check despite the PR's stated requirement to reject targets outside writablePT_LOADsegments.Keep the zero-flags shortcut only for calls that request no permission check. For example:
if (header.Type == PT_LOAD && (flags == 0 || (header.Flags & flags) == flags) && address >= header.MappedAddress && address - header.MappedAddress < header.MemorySize && size <= header.MemorySize - (address - header.MappedAddress))Update the flagless relocation test to expect rejection, and retain a separate containment-only test if that behavior is needed.
Non-blocking
None identified.
Sources
- Linux kernel ELF permission mapping at commit
ffc253263a1375a65fa6c9f62a893e9767fbebfa - Changed
RequireLoadedcheck at reviewed head - Flagless relocation test at reviewed head
Not verified
No local build or C++ tests. Hosted relinker jobs had not completed. The local precheck's ABI/test-quality helper commands were unavailable from its isolated snapshot; that is a tooling limitation, not a PR test result.
To get this to a good state
Require PF_W for writable relocation targets even when the segment has no flags, and change the flagless case to assert rejection. I will rerun the same checks and inspect the results on the new head.
|
|
MarcZX10
left a comment
There was a problem hiding this comment.
Re-checked — same head, looks good
I’m correcting my earlier change request: zero-flag PT_LOAD segments are normalized to RWX before the Linux output ELF is written, so the relocation case I previously flagged does target a writable output mapping.
What I ran
Reviewed head 50b37bb1a98edde1d306775c870a20440fd26814 against base 73f4f7e06e4f210a8c6570e335fd654dc0a86134; this is the same code head as my earlier review. The live merge preview was clean. Environment: Linux x86_64, Python 3.11.2, GCC 12.2.0. No local build or compilation was run.
git diff --check 73f4f7e06e4f210a8c6570e335fd654dc0a86134...50b37bb1a98edde1d306775c870a20440fd26814
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 13977
python3 -m py_compile core/relinker/relinker/tests/test_game_patches.py core/relinker/relinker/tests/test_guest_symbol_names.py core/relinker/relinker/tests/test_import_symbol_types.py core/relinker/relinker/tests/test_linux_load_bounds.py core/relinker/relinker/tests/test_unused_filter_indirect.py
python3 tools/check_test_registration.py --scripts
python3 /marc/AnyPS5/tools/pr_test_quality.py --root /tmp/anyps5-review-13977 --json core/relinker/relinker/tests/test_linux_load_bounds.py core/relinker/relinker/tests/test_game_patches.py core/relinker/relinker/tests/test_guest_symbol_names.py core/relinker/relinker/tests/test_import_symbol_types.py core/relinker/relinker/tests/test_unused_filter_indirect.py
gh pr checks 13977 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff check, conventions and test registration | Passed. |
| Python syntax | Passed for all five changed Python test modules. |
| Test-quality heuristic | Emitted five generic constant-assertion prompts. They point to assertions outside the changed hunks; manual diff review found no new assertion that only checks a constant. |
| Precheck runner | Partial: the isolated head worktree lacks the local-only pr_test_quality.py helper. The check did not run there; I ran that helper separately against the exact head. |
| Hosted relinker checks | Ubuntu/g++, Windows/g++, macOS/clang++ x86_64, macOS/clang++, and ASAN passed. Generic linux, windows, register-context, and write-watch jobs were skipped. |
| Source review | The RequireLoaded exception for zero flags matches the actual output transformation; the earlier blocking concern is resolved by existing code, not by a new PR commit. |
Selected unedited check output
All conventions checks passed.
230 test script(s), each run by an add_test or a workflow
/usr/bin/python3: can't open file '/tmp/anyps5-pr-precheck/boykopovar-AnyPS5-pr-13977-50b37bb1a98e/head-50b37bb1a98e-3dfcc8cc/tools/pr_test_quality.py': [Errno 2] No such file or directory
{
"findings": [
{
"path": "core/relinker/relinker/tests/test_game_patches.py",
"line": 57,
"kind": "constant-assertion",
"message": "Assertion appears to check only a constant; inspect manually"
},
{
"path": "core/relinker/relinker/tests/test_guest_symbol_names.py",
"line": 208,
"kind": "constant-assertion",
"message": "Assertion appears to check only a constant; inspect manually"
},
{
"path": "core/relinker/relinker/tests/test_guest_symbol_names.py",
"line": 215,
"kind": "constant-assertion",
"message": "Assertion appears to check only a constant; inspect manually"
},
{
"path": "core/relinker/relinker/tests/test_guest_symbol_names.py",
"line": 266,
"kind": "constant-assertion",
"message": "Assertion appears to check only a constant; inspect manually"
},
{
"path": "core/relinker/relinker/tests/test_unused_filter_indirect.py",
"line": 71,
"kind": "constant-assertion",
"message": "Assertion appears to check only a constant; inspect manually"
}
],
"negative_control_candidates": [],
"limits": [
"Heuristics can miss weak assertions and can flag valid tests; inspect every finding.",
"A test that passes against base is not by itself proof that the PR test is inadequate.",
"Negative controls are only candidates until the test actually runs against base and fails by assertion rather than import/setup error."
]
}
conventions pass 16s https://github.com/boykopovar/AnyPS5/actions/runs/41214094282/job/124574898848
eligibility pass 7s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562241307
intake pass 4s https://github.com/boykopovar/AnyPS5/actions/runs/41209812774/job/124562236959
labels pass 12s https://github.com/boykopovar/AnyPS5/actions/runs/41214094327/job/124574899024
relinker (macos-latest, clang++) pass 4m34s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562276852
relinker (macos-latest, clang++, x86_64) pass 5m48s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562276874
relinker (ubuntu-latest, g++) pass 3m38s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562276878
relinker (windows-latest, g++) pass 9m5s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562276911
relinker-asan pass 2m48s https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562276808
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562277591
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562277957
windows skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562277605
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41209814310/job/124562277787
Findings
Blocking
None confirmed after reviewing the author’s evidence and the unchanged implementation path.
Non-blocking
I did not run a local negative control. The macOS ARM relinker job passed during this re-check; the previously reviewed CI jobs also passed at the current head.
Sources
RequireLoadedchecks the original input headers.WriteLayoutapplies_fixLoadFlagsto every keptPT_LOADbefore serializing it;_fixLoadFlagsmaps zero flags toPF_R | PF_W | PF_X.- The
relative-in-flaglesscase covers the transformed segment in normal, fixed-base, and lazy-binding modes. - The pipeline skips
R_X86_64_NONEbefore constructing relocation tables for the Linux patcher.
Not verified
I did not build or run tests locally. The hosted relinker CI passed on Ubuntu, Windows, macOS x86_64, macOS ARM, and ASAN; generic platform jobs listed above were skipped. No local runtime or title integration was tested.
What
On 60bdb06 (line numbers below refer to it) the Linux patcher checks every
DT_RELAandDT_JMPRELr_offsetwithRequireLoaded(LinuxElfPatcher.cpp:115-120) but passes no required flags (line 427), so a relocation may target aPT_LOADwithoutPF_W. The rebuilt Linux dynamic section never declares text relocations: it takes onlyDT_NEEDEDfrom the input, so an inputDT_TEXTRELis dropped, and itsDT_FLAGSisDF_BIND_NOWwithout--lazy-bindingand absent with it. The gABI allows a relocation to modify a non-writable segment only whenDT_TEXTRELis present, and glibc's_dl_relocate_object(elf/dl-reloc.c) makes such segments writable only for an object withDT_TEXTREL; this comes from reading the glibc source, not from running an output. For aR_X86_64_RELATIVE,R_X86_64_64orR_X86_64_GLOB_DATinDT_RELAor anR_X86_64_JUMP_SLOTinDT_JMPRELwhoser_offsetlies in the fixture'sPF_R | PF_XPT_LOAD, Linux and--lazy-bindingoutput were written with exit 0; aRELATIVEcase withDT_TEXTRELin the input converted the same way, without the tag in the output. The same holds for the lazyDT_PLTGOT, where ld.so stores its link map and resolver (elf_machine_runtime_setup):RequireLoadedchecks it with no required flags as well (line 429), so aDT_PLTGOTinside code converted with--lazy-binding. The other targets already refuse most of these:--macosrefuses all four (Mach-O fixup target is not in a writable segment,MacOsImage.cpp:227, orRuntime import target is not writable), and--windowsrefuses the three that bind imports (WindowsLoadImage.cpp:154) and accepts only theRELATIVE, which becomes a PE base relocation that the Windows loader applies to read-only pages itself. The guest image reader already refusesDT_TEXTRELandDF_TEXTRELin bundled modules (GuestImageReader.cpp:87-90).RequireLoadednow getsPF_Wfor everyDT_RELAandDT_JMPRELentry the patcher writes, and forDT_PLTGOTunder--lazy-binding, so such an input fails at relink time withRELA r_offset 0x300 (8 bytes) does not fit inside a writable PT_LOAD segment(orJMPREL …,DT_PLTGOT …) instead of in the dynamic loader.R_X86_64_NONEentries never reach the patcher (RelinkerPipeline.cpp:395-396), and without--lazy-bindingDT_PLTGOTstill only has to lie in aPT_LOAD. APT_LOADwhosep_flagsis 0 still passes, as it does fore_entry(line 117): the Linux output maps itPF_R | PF_W | PF_X(_fixLoadFlags,ProgramHeaderLayoutBuilder.cpp:126-127, from bc14e47), so the kernel'smake_protgives the relocation target a writable page;entry-in-flagless-loadchecks those output flags. The message for anr_offsetoutside everyPT_LOAD, and for a lazyDT_PLTGOTthere, now also names a writable segment.DT_TEXTRELthrough. Passing it through would make ld.so remap code pages writable at load, while macOS output and bundled modules already refuse the same inputs. On fix(relinker): reject macOS fixups outside writable segments #11045 wachi35676 mapped all 129,623RELATIVE,R_X86_64_64,GLOB_DATandJUMP_SLOTtargets of 24 PS5 ELF files (the homebrew eboots of PPSA99005, PPSA47402, PPSA99581 and PPSA99995, theirsce_modulePRX files and 16 wine PRX modules of PPSA99995) to theirPT_LOADand found every one in aPF_Wsegment.linux_load_boundsgains cases forRELATIVEandR_X86_64_32into aPF_R | PF_XPT_LOAD, aRELAinto a read-only.bss-likePT_LOAD, aJMPRELinto code, and accepted cases forRELATIVEinto anRWXand a flaglessPT_LOAD, each for the default,--linux-fixed-base 0x800000and--lazy-binding, plus aDT_PLTGOTinside code that converts eagerly and fails with--lazy-binding; it andunused_filter_indirectexpect the new message text. Six existing tests placed GOT slots or relocation targets in the singlePF_R | PF_XPT_LOADofmain_fixtureor of the C++ executable fixture and now fail by design; their fixtures give these targets a writable segment:importing_executableintest_guest_symbol_names.py(only when it has imports, becauseinit_array_rootscounts the code regions of the import-free fixture) and the separate helper of the same name intest_import_symbol_types.pysplit thePT_LOADintoPF_R | PF_Xat 0x4000 andPF_R | PF_Wat 0x4800, because the strict filter refuses a writable code segment (StrictNidFilter.cpp,unsupported executable segment layout);test_game_patches.pyandRelocTableBoundsTests.cppsetPF_Won their existingPT_LOAD.main_fixtureitself is unchanged.pr_overlap.py --branchreports no overlap with open pull requests. A trial merge with #10917, which also changesLinuxElfPatcher.cpp, and with #11487 is clean.Tested
Windows 11, WinLibs GCC 15.2.0, relinker-only build (
-DANYPS5_RELINKER_ONLY=ON, Ninja, Release), on 50b37bb (based on 60bdb06;mainat 73f4f7e changed none of these files and merges cleanly):LinuxElfPatcher.cppof 60bdb06, the four relocation types above into aPF_R | PF_XPT_LOADconvert with exit 0 for Linux and--lazy-binding, and theRELATIVEcase with aDT_TEXTRELin the input converts withoutDT_TEXTRELin the output and withDT_FLAGS0x8; with this change all of them fail with the new message,--windowsand--macosbehave as described above, and the same input with anRWXPT_LOADstill converts.guest_symbol_names,guest_registry,import_symbol_types,duplicate_relocations,game_patchesandreloc_table_boundswithdoes not fit inside a writable PT_LOAD segment; with them these six,linux_load_bounds,unused_filter_indirect,init_array_roots,needed_libraries,guest_deferred_dependencies,sce_code_gapsandsce_module_code_extentpass.ctest --test-dir build-relinker -j 5on 50b37bb: 543 tests, 537 passed,long_pathsskipped;annotate_module, a Python test oftools/annotate_module.pythat does not run the relinker, timed out under the parallel load and passed alone in 15.5 to 16 s in three of four runs on this machine;select_testsand its three partsselect_tests_SelectGraphStateTests,select_tests_SelectBuildTestsandselect_tests_SelectArgumentTestsfail with the Windows symlink fixture'sFileExistsError, as onmainon this machine; this change touches nothing undertools/.python tools/check_conventions.py --base 60bdb06135with this title and description passed on 50b37bb.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)