Skip to content

fix(relinker): reject unsigned pointer overflow during relative unwind decoding - #10796

Closed
slopgobblercobbler wants to merge 1 commit into
boykopovar:mainfrom
slopgobblercobbler:fix/relinker-unwind-unsigned-pointer-overflow
Closed

slopgobblercobbler wants to merge 1 commit into
boykopovar:mainfrom
slopgobblercobbler:fix/relinker-unwind-unsigned-pointer-overflow

Conversation

@slopgobblercobbler

Copy link
Copy Markdown
Contributor

What

Bounds check relative displacements during unwind decoding in EhFrameReader.cpp when handling unsigned encodings (such as DW_EH_PE_pcrel | DW_EH_PE_udata8), preventing silent integer overflow and wrap-around. Add bounds checking unit test case in UnwindRecordBoundsTests.cpp.

Tested

Windows 11, unwind_record_bounds_tests (all 15 unit test cases pass).

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • Fixes: none
  • Unblocks: none
  • GPUs tested: n/a
  • AI-assisted: yes

@MarcZX10 MarcZX10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new overflow guard fixes the pcrel|udata8 case, but its signedness test also classifies DW_EH_PE_absptr as signed ((encoding & 0x0F) == 0). The DWARF EH encoding table defines absptr as a literal pointer format; the value-format table distinguishes unsigned udata*/uleb128 from signed sdata*/sleb128. For an unsigned pcrel|absptr value with bit 63 set and a high base, this treats the value as negative and can allow base + value to wrap, defeating the new overflow check. Please add a regression for the high-base pcrel|absptr wrap and classify signedness from the signed value formats only (or document a PS5-specific encoding rule). I verified this against the Linux Foundation DWARF EH encoding tables. I have not run the Windows-only test locally; the relinker CI jobs currently pass.

@roboterspinne roboterspinne left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed head 53c6427 merged onto upstream/main 4017310 (2026-10-25); the merge is clean. It is also clean onto the current main e05ba6f, whose newer commits touch none of these files.

Environment: Windows 11 Home 10.0.26200, Intel i9-12900H, WinLibs GCC 15.2.0 UCRT, CMake 4.3.1, Ninja 1.13.2, -DANYPS5_RELINKER_ONLY=ON -DBUILD_TESTING=ON, Release.

Results

  • Full relinker-only suite on the merged tree: 292 tests, 290 pass, long_paths skipped. The one failure is patch_advisor_unit. It fails the same way on plain main on this machine, because the temp path contains a space (#10667), and it is unrelated. unwind_record_bounds passes.
  • Counter-check: with main's EhFrameReader.cpp under this test file, unwind_record_bounds_tests still prints 15 unwind record cases passed. On main, case 10 is rejected for an unrelated reason. Its FDE pointer at offset 24 stays the absolute 0x1060, is read as pcrel and resolves to 0x2078, so main throws unwind address is not file-backed before the wrap is ever reached. The new case therefore passes with or without the change.
  • To compare behaviour, I linked a probe once against main's reader and once against this branch's, and ran the same inputs through both. Signed formats (0x1b, 0x1c, 0x3b, 0x3c) behave the same on both readers. The differences are the ones in the inline comment, plus one useful case. With the header at 0xffffffffffff0000, a 0x13 pcrel|udata4 value of 0x107f0 carries to 0x800. main accepts it and this branch rejects it. That would be a regression test that actually pins an unsigned carry check, without touching the #5773 behaviour.
  • check_conventions.py --base upstream/main passes with this title and body.

Should fix

  • See the inline comment on line 139: the change rejects 0x34/0x14 again, and it starts rejecting high absptr targets that main accepts.
  • Case 10 (line 59) does not test the change. A case that main accepts and this branch rejects would pin it, for example the 0x13 high-base carry above. Accept cases for 0x34 and 0x14 below the base would also keep #5773's behaviour pinned.

Not checked: the PS5 SDK's own .eh_frame_hdr encodings; Linux and macOS locally.

I'd hold this until the rule for pointer-width formats is settled. For disclosure: I have open relinker PRs (#10971, #10947, #10946, #10945, #10752, #10732, #10731, #10730, #10712, #10679, #10607, #10564, #10403, #10402, #10401, #10391, #10390) plus #10667 on tools/tests/test_suggest_patches.py; none touches the unwind reader or its tests.

Written with AI assistance (Claude Code). I ran the build, the tests, the counter-check and the probes above on my Windows 11 machine.

if (negative ? base < std::uint64_t{0} - value : value > std::numeric_limits<VirtualAddress>::max() - base)
throw RelinkerException("Strict filter: unwind pointer address overflowed", field);
}
const bool isSigned = (encoding & 0x08) != 0 || (encoding & 0x0F) == 0;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This brings back the rejection that #5773 removed. On #5773, wachi35676 showed that 8-byte relative values pointing below their base must decode modulo 2^64, because that is how the unwinders add them. libgcc (unwind-pe.h, read_encoded_value_with_base) and LLVM libunwind (AddressSpace.hpp, getEncodedP) both add the base as an unsigned pointer-width result += base. That covers 0x30, 0x34, 0x10 and 0x14 targeting 0x800 from a 0x1000 base. The guard was then limited to signed encodings, the "below" test case was added for 0x30, and adriadam10 merged it.

With this line, only the absptr forms (0x30, 0x10) still pass, because absptr is now counted as signed. The udata8 forms of the same bytes are rejected. I took the test's below fixture (function segment at 0x800, header at 0x1000) and ran each reader on the same input:

table encoding stored value main this branch
0x34 datarel|udata8 0xfffffffffffff800 [0x800,0x810) unwind pointer address overflowed
0x14 pcrel|udata8 0xfffffffffffff7f0 [0x800,0x810) unwind pointer address overflowed
0x30 datarel|absptr, high target 0xfffffffffffff010 0xffffffffffffe010 accepted unwind pointer address overflowed
0x10 pcrel|absptr, high target 0xfffffffffffff010 0xffffffffffffe000 accepted unwind pointer address overflowed

The change request is right on two points. The LSB table describes absptr as "a literal pointer", so there is no basis for treating it as signed. And a high-base pcrel|absptr wrap does get through here: header 0x8000000000000000, value 0x80000000000007f0, accepted with target 0x800. But for a pointer-width value, checking it as signed and checking it as unsigned are exact opposites. Every value with bit 63 set passes one check and fails the other. So any check on absptr or udata8 rejects either the #5773 below case or a valid high target. That is also why the rule requested in the change request (isSigned = (encoding & 0x08) != 0) fails the existing below case with unwind pointer address overflowed.

Suggestion: keep absptr and udata8 modular, as #5773 decided. If you want a carry check, apply it only to unsigned formats narrower than the address (udata2, udata4), where a carry cannot express a negative offset.

@slopgobblercobbler

Copy link
Copy Markdown
Contributor Author

Closing this PR per reviews from @MarcZX10 and @roboterspinne: As established in #5773, relative unwind pointer encoding frequently utilizes pointer-width encodings (such as \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants