Skip to content

fix(libkernel): fail the signal action, mask, stack and pending calls with EFAULT on an unusable pointer - #11273

Merged
adriadam10 merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/signal-pointer-faults
Oct 27, 2026
Merged

adriadam10 merged 1 commit into
boykopovar:mainfrom
Gruzzberry:fix/signal-pointer-faults

Conversation

@Gruzzberry

@Gruzzberry Gruzzberry commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

What

sigaction, sigprocmask (and _sigprocmask, which pthread_sigmask also reaches), sigaltstack and sigpending read and wrote their guest pointers directly, so an unreadable or unwritable one crashed the host instead of failing. FreeBSD 11.4's sys_sigaction, sys_sigprocmask, sys_sigaltstack and sys_sigpending copy the new value in first (copyin, EFAULT), then validate it (EINVAL for a bad signal number, how, or flags; ENOMEM for a small stack), apply it, and only then copy the old value out (EFAULT, with the change already made). The calls now do the same with the new GuestCopyIn_nid_no_patch (the read counterpart of GuestCopyOut_nid_no_patch) and GuestCopyOut_nid_no_patch. The old mask is stored before the handlers of newly unblocked pending signals run, as on FreeBSD. pthread_sigmask returns the error number (EFAULT) rather than -1, leaves errno as it was, and no longer checks how before the kernel path reads the set, so a bad how with an unreadable set gives EFAULT as in sys_sigprocmask. Results and errnos for readable pointers are unchanged. sigsuspend is left alone: its null-mask throw is asserted by guest_signals.

Tested

Windows 11 (10.0.26200), WinLibs MinGW-w64 GCC 15.2.0 r7, Ninja, Release, -DBUILD_TESTING=ON, on current main.

  • guest_signals: new CheckSignalPointerFaults for an unreadable 0x10 action, set and stack and an unreadable or read-only old value (EFAULT), the signal number 0 and how 9 with an unreadable input (EFAULT ahead of EINVAL, pinning the order) and with a readable one (EINVAL), a sigprocmask whose old value is unwritable still applying the block, and a null and unwritable sigpending buffer, and pthread_sigmask returning 14 for an unreadable set (also with a bad how) and for a read-only old mask but 22 for a bad how with a readable set. On main the test segfaults; with this change it passes, three runs out of three.
  • Full suite: the only failures are the known environment ones (pr_overlap_encoding, agc_host_import_cut_lifetime, agc_driver_depth_plane_partial_store).
  • python tools/check_conventions.py --base upstream/main: passes.
  • Not tested: Linux, a console, a title run.

Overlaps

Checklist

AI-assisted (Claude Code).

🤖 Generated with Claude Code

@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 raw sigprocmask path returns -1 and sets errno = EFAULT for an unreadable set or previousSet. pthread_sigmask_nid_postfix forwards that result directly, but the POSIX thread API returns the error number itself (for example, EFAULT/14), not -1; FreeBSD's _pthread_sigmask returns errno when its internal operation fails. The new tests cover only sigprocmask_nid_postfix, so they miss this wrapper-specific behavior. Please translate the checked-copy failures to the direct error result in the pthread_sigmask wrapper and add tests for unreadable input/output pointers there, including the wrapper's ordering when how is invalid. I ran exact-head git diff --check and tools/check_conventions.py --base upstream/main; both pass. Hosted policy checks pass, but all cross-platform relinker/sanitizer jobs are still running and full product jobs are skipped; the Windows focused tests are author-reported.

@Gruzzberry
Gruzzberry force-pushed the fix/signal-pointer-faults branch from 85b78f0 to 35f349e Compare October 25, 2026 23:24
@Gruzzberry
Gruzzberry marked this pull request as ready for review October 25, 2026 23:26

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

Re-reviewed exact head 35f349ed81852e64ee17f8068526f1bfb551f0be. The new pthread_sigmask error-number translation fixes the prior -1/errno issue, but its invalid-how fast path still runs before reading set: pthread_sigmask_nid_postfix(9, unreadable, nullptr) returns 22, and the new test codifies that result. This conflicts with the PR’s stated FreeBSD ordering. In FreeBSD 11.4, __thr_sigprocmask copies a non-null set for every how except SIG_UNBLOCK, then calls __sys_sigprocmask; the kernel sys_sigprocmask copies in the set before kern_sigprocmask validates how. For how == 9 and an unreadable set, that path therefore returns EFAULT (14). Please remove/defer the wrapper precheck for this case and change the regression to expect 14; keep the readable invalid-how case expecting 22. Source: https://github.com/freebsd/freebsd-src/blob/release/11.4.0/lib/libthr/thread/thr_sig.c and https://github.com/freebsd/freebsd-src/blob/release/11.4.0/sys/kern/kern_sig.c. I ran exact-head git diff --check upstream/main...HEAD and tools/check_conventions.py --base upstream/main; both pass. Hosted policy checks pass, but the refreshed relinker/sanitizer/macOS jobs are still pending, product jobs are skipped, and the focused Windows run is author-reported rather than independently run here.

@MarcZX10

Copy link
Copy Markdown
Contributor

One separate checklist inconsistency: the Overlaps section says this PR must follow #11264 because both branches add the identical GuestCopyIn_nid_no_patch declaration/definition, but the checklist currently says Depends on: none. CONTRIBUTING.md asks for the prerequisite on that checklist line when merge order is required. Please list #11264 there (and any other PRs that truly must land first) so the merge-order constraint is machine- and reviewer-visible.

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

Re-reviewed exact head fc123bf178c762ab7a9f467c7478b54a07a3c960. The wrapper now copies set before validating how, and the new pthread_sigmask(9, unreadable, nullptr) == EFAULT regression addresses my prior FreeBSD-ordering finding. The remaining blocking issue is the checklist: the Overlaps section says this PR must follow #10975, #3986, #10749, #11002 and #11264, but Depends on still says none. Please put the required merge-order PRs on that checklist line (or revise the overlap statement if any are no longer prerequisites). I reviewed the current diff; hosted conventions and policy checks pass, cross-platform relinker jobs are still running, product jobs are skipped, and the Windows focused tests remain author-reported rather than independently run.

@Gruzzberry
Gruzzberry force-pushed the fix/signal-pointer-faults branch from fc123bf to 25d53dd Compare October 25, 2026 23:41
@Gruzzberry

Copy link
Copy Markdown
Contributor Author

Fixed in 25d53dd: pthread_sigmask no longer checks how before the kernel path, so a bad how with an unreadable set gives 14 and with a readable set 22; it also leaves errno unchanged, which guest_signals asserts. The regression now expects 14 for that case. guest_signals passes 3/3 and so do the other signal tests.

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

Re-reviewed exact head 25d53dd03f9ddf1d6a0004202ee8d7df7f63d8c6. The new pthread_sigmask ordering and error-number behavior address my prior code findings. One blocking checklist mismatch remains: the Overlaps section says this PR must follow #10975, #3986, #10749, #11002 and #11264, but the checklist still says Depends on: none. Please list the actual required merge-order PRs on the checklist line, or revise the overlap section if any are no longer prerequisites. Current conventions and policy checks pass; cross-platform relinker/sanitizer jobs are still running, and Linux/Windows product jobs are skipped by the workflow. The focused Windows tests are author-reported, not independently run here.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

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.

Pull request Shared files not compared
PR 3986 core/libs/prx/libkernel/System/src/Signals.cpp
core/libs/tests/GuestSignals.cpp
PR 5021 core/libs/prx/libc/src/General.cpp
PR 9371 core/libs/prx/libc/src/General.cpp
PR 9389 core/libs/prx/libkernel/System/src/Signals.cpp
PR 9447 core/libs/prx/libc/include/General.hpp
PR 9929 core/libs/prx/libc/include/General.hpp
PR 10749 core/libs/prx/libc/include/General.hpp
core/libs/prx/libkernel/System/src/Signals.cpp
core/libs/tests/GuestSignals.cpp
PR 11002 core/libs/prx/libkernel/System/src/Signals.cpp
core/libs/tests/GuestSignals.cpp

@Gruzzberry

Copy link
Copy Markdown
Contributor Author

Checklist fixed: Depends on: #10975 with the remaining merge order after it.

@wachi35676

Copy link
Copy Markdown
Contributor

Windows and Linux runs of head 25d53dd03f9d merged onto main 0f34ebb85. Windows used WinLibs GCC 15.2.0, Linux used WSL with GCC 13; both Release. The description lists Linux as not tested.

AI-assisted: yes (Claude Code); results above were run on this machine.

@harisibrar1

Copy link
Copy Markdown
Contributor

@Gruzzberry heads-up: this PR and #10501 both add a trailing optional parameter to ChangeBlockedMask in Signals.cpp. #10501 adds bool* caught, set when DeliverUnblockedPending (which now returns bool) ran a guest handler, for pselect's SetSignalMaskCatching_nid_no_patch. This one adds GuestSignalSet* guestPrevious. git merge-tree conflicts on the signature line and on the delivery line at the end of the function; nothing else overlaps.

Whichever lands second keeps both parameters, e.g. (int how, const GuestSignalSet* set, GuestSignalSet* previousSet, bool releaseHolds, GuestSignalSet* guestPrevious = nullptr, bool* caught = nullptr), with the single caller that passes caught updated. If #11273 lands first I'll do that in #10501.

Reviewed with AI assistance (Claude Code); the checks above were run locally on my machine.

… with EFAULT on an unusable pointer

FreeBSD 11.4's sys_sigaction, sys_sigprocmask and sys_sigaltstack copy
their input in before validating it and copy the old value out after
the change, and sys_sigpending copies out; a bad pointer gives EFAULT.
The guest pointers were dereferenced directly.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@Gruzzberry
Gruzzberry force-pushed the fix/signal-pointer-faults branch from 25d53dd to 1fbf3a4 Compare October 26, 2026 09:30
@github-actions github-actions Bot removed the needs rebase Conflicts with main, rebase needed label Oct 26, 2026
@adriadam10 adriadam10 self-assigned this Oct 27, 2026
@adriadam10
adriadam10 dismissed MarcZX10’s stale review October 27, 2026 07:41

Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.

@adriadam10
adriadam10 merged commit a1053d9 into boykopovar:main Oct 27, 2026
17 checks passed
@adriadam10

Copy link
Copy Markdown
Collaborator

Checked the five calls against FreeBSD 11.4 sys_sigaction, sys_sigprocmask, sys_sigaltstack and sys_sigpending (copyin first, then validation, change applied, copyout last), pthread_sigmask returning the error number with errno untouched, and that CheckSignalPointerFaults segfaults on main and pins the 14/22 ordering; GuestCopyIn_nid_no_patch is identical to the one in #10975/#11264; merged.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants