Skip to content

fix(relinker): reject prefixed system-call instructions - #1935

Merged
oneandonlydean merged 1 commit into
boykopovar:mainfrom
Zinebyte:fix/prefixed-syscall-validation
Oct 10, 2026
Merged

oneandonlydean merged 1 commit into
boykopovar:mainfrom
Zinebyte:fix/prefixed-syscall-validation

Conversation

@Zinebyte

@Zinebyte Zinebyte commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

What

The relinker checked the first two bytes of each decoded instruction for forbidden system calls. A prefix hid the opcode: 48 0F 05 passed validation and produced an executable even though it contains SYSCALL.

Check the decoded instruction's OpcodeOffset() instead. Prefixed SYSCALL, SYSENTER, SYSRET and INT 80h now fail with the existing error at the instruction's start address. Opcode-like bytes in immediates remain accepted, and --skip-syscall-check retains its behavior.

This addresses the scanner follow-up explicitly left out of #211. No open overlapping fix was found before implementation or on the final search. CallSiteResolver remains outside this change.

Tested

Zorin OS 18.1, Linux x86-64, GCC 13.3.0, CMake 3.28.3, Release, relinker-only build at f145139676a48306681d3a4d5fabf83938759daf. Build parallelism: 1.

  • Expanded executable_segments regression fails on the upstream binary: the REX-prefixed syscall is accepted and output is written. It passes with the fix in 0.03 s.
  • Covers eight prefixed forms across all four forbidden instructions, the instruction-start diagnostic 0x1801, no output on rejection, immediate-byte decoys, and the explicit skip option.
  • ctest --test-dir build-relinker --output-on-failure --parallel 1: 38/38 passed in 6.44 s; longest test 5.28 s.
  • python3 tools/check_conventions.py --base origin/main and git diff --check: passed.
  • Independent read-only correctness and portability review: no findings.

Synthetic inputs only. No title or hardware compatibility testing. Windows PE output is inspected on Linux; native Windows/macOS execution and cold shared CI timing were not measured.

Checklist

  • Based on current main; no open overlapping implementation found
  • One topic per PR
  • No added code comments
  • Unsupported paths still throw; no silent stubs added
  • No investigation files, logs or images added to the repository
  • No new third-party dependencies
  • General behavior, no title-specific changes
  • Depends on: none
  • AI-assisted: yes (Codex). The submitting author is responsible for every changed line.

Use the decoded opcode offset when checking forbidden system calls.
Add synthetic regressions for prefixed forms, immediate decoys, and the skip option.

AI-assisted: Codex. The submitting author is responsible for every changed line.
@Zinebyte
Zinebyte marked this pull request as ready for review October 9, 2026 02:19

@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.

Reviewed the full two-file diff at 2381e7f. Using the decoder-reported OpcodeOffset makes the four syscall signatures visible after prefixes while preserving the instruction-start diagnostic; the tests also check immediate-byte decoys and the skip option. The Actions API shows this exact head has Build and Conventions runs marked action_required with no jobs, so upstream CI has not run. CONTRIBUTING requires those checks before merge; please request maintainer approval for the fork workflows. I am holding approval until those final checks run.

@rmz-24

rmz-24 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

Windows run, since the description notes it wasn't tested there: Windows 11 Pro 25H2 (10.0.26200.9457), MinGW-w64 GCC 15.2.0 (WinLibs r7), Release, full build with BUILD_TESTING=ON. 2381e7f cherry-picked onto current main d70b899 (applies cleanly).

  • executable_segments passes.
  • Full ctest: 510/510, including the Windows PE execution tests.

AI-assisted: yes

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.

Pull request Conflicting files (lines on main) Same exports Same new files
#2206 core/relinker/relinker/src/analysis/SyscallScanner.cpp L2-3, L34-52

@StarMrGamer

Copy link
Copy Markdown
Contributor

Verified this against a clean main build (d70b8998, relinker-only, GCC 16.2.1) — the hole is real, not theoretical.

Fixture whose first executable-segment bytes are the instruction, same input through both relinkers:

bytes main your head 2381e7fc
0f 05 c3 rejected rejected
66 0f 05 c3 accepted rejected
48 0f 05 c3 accepted rejected
67 0f 05 c3 accepted rejected

main produces an output for all three prefixed forms: the scan compares the first two bytes of the instruction, so a prefix hides the opcode and the syscall is never seen at all. Reading at DecodedInstruction::OpcodeOffset() is the right fix, and your head builds clean with ctest 38/38 here.

One thing worth knowing before either of us lands: #2206 changes the same lines of ScanCodeSectionForSyscalls. It adds a classification step that also reads the instruction's bytes, and it currently reads them at the instruction start. So the conflict resolution has to keep both behaviours:

The composed form classifies at i + opcodeOffset too. Whichever lands second needs the rebase; #2206 is the smaller one to adapt, and I am happy to rebase it onto this.

@Zinebyte

Zinebyte commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks rmz-24 for the Windows validation: executable_segments and the full 510/510 CTest pass on Windows 11 / WinLibs GCC 15.2, including Windows PE execution, with 2381e7fc cherry-picked onto d70b8998. Upstream Build and Conventions have now succeeded on this PR's exact head.

Thanks StarMrGamer for checking the prefixed cases and the composition requirement. #2206 adds syscall-number diagnostics in the shared scan loop; it does not replace this PR's prefix-aware OpcodeOffset() detection fix. Neither functionally depends on the other, so Depends on: none remains accurate. Conflict resolution needs classification at the decoded opcode position while diagnostics retain the instruction-start address, preserving both behaviors.

AI-assisted review and reply.

oneandonlydean added a commit to oneandonlydean/AnyPS5 that referenced this pull request Oct 10, 2026
oneandonlydean added a commit to oneandonlydean/AnyPS5 that referenced this pull request Oct 10, 2026
@oneandonlydean
oneandonlydean merged commit 300bd79 into boykopovar:main Oct 10, 2026
7 checks passed
@nyavana

nyavana commented Oct 10, 2026

Copy link
Copy Markdown
Contributor

@Zinebyte Would these 24 additional boundary tests be useful? nyavana@ac6381e

Linux CTest: 66/66 passed, with one internal clang-dependent skip. Two mutation checks verified the added coverage. AI-assisted.

StarMrGamer added a commit to StarMrGamer/AnyPS5 that referenced this pull request Oct 10, 2026
The lookback classifies preceding instructions at their decoded opcode
offset, the same place the forbidden-instruction check looks since boykopovar#1935,
so a legacy-prefixed mov rax, imm or mov r10, rcx is recognized. The REX
byte stays part of the patterns, so mov r8 and mov r8d cannot masquerade
as rax loads. Anything unrecognized still stops the inference.

Adds prefixed fixtures to both test suites: a prefixed constant stub that
must report syscall number 9, and a prefixed clobbered stub that must
report no number.

AI-assisted: yes. The submitting author is responsible for every changed line.
SleglJan pushed a commit to SleglJan/AnyPS5 that referenced this pull request Oct 10, 2026
…-validation

fix(relinker): reject prefixed system-call instructions
@Zinebyte

Copy link
Copy Markdown
Contributor Author

Thanks @nyavana, these look useful! The exact-boundary, out-of-segment byte and truncated-instruction cases add useful coverage. Since #1935 is now merged, a focused test-only follow-up against current main would make sense.

The diff was reviewed with AI assistance; I haven't independently run the tests. Appreciate you adding these.

adriadam10 pushed a commit to StarMrGamer/AnyPS5 that referenced this pull request Oct 10, 2026
The lookback classifies preceding instructions at their decoded opcode
offset, the same place the forbidden-instruction check looks since boykopovar#1935,
so a legacy-prefixed mov rax, imm or mov r10, rcx is recognized. The REX
byte stays part of the patterns, so mov r8 and mov r8d cannot masquerade
as rax loads. Anything unrecognized still stops the inference.

Adds prefixed fixtures to both test suites: a prefixed constant stub that
must report syscall number 9, and a prefixed clobbered stub that must
report no number.

AI-assisted: yes. The submitting author is responsible for every changed line.
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.

6 participants