Repository navigation
fix(libkernel): fail gettimeofday, getitimer, setitimer and the sceKernel time getters with EFAULT on an unusable pointer - #10975
Conversation
…rnel time getters with EFAULT on an unusable pointer These wrote their result through the caller's pointer in place, or read the itimerval of setitimer without a check, so a pointer into unmapped memory crashed the host. Build the result locally and copy it out with the checked write (and read the setitimer value with a checked read), failing with EFAULT as FreeBSD 11.4 does: copyout in sys_gettimeofday (sys/kern/kern_time.c:646 and 651) and sys_getitimer (kern_time.c:745), copyin (kern_time.c:803) and copyout (kern_time.c:808) in sys_setitimer. getrlimit and getrusage are left to boykopovar#8986 and boykopovar#8988, which cover the same family for those two calls. Builds on fix/output-structs-efault, which adds GuestCopyOut_nid_no_patch. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Requesting changes: GuestCopyIn_nid_no_patch has checked-copy implementations for Windows and Linux, but on macOS it falls through to the unconditional std::memcpy at core/libs/prx/libc/src/General.cpp:865-867. The new setitimer path calls this helper for the guest value pointer. Therefore the PR's setitimer_nid_postfix(0, badTimer, ...) regression case will dereference the deliberately unmapped pointer and fault the host on macOS instead of returning -1/EFAULT. GuestCopyOut_nid_no_patch already has a macOS mach_vm_write path, but the read helper does not have the corresponding checked read.
Please add a checked macOS read (for example using the appropriate Mach VM read API and validating that the full requested byte count was copied), or use an existing cross-platform guest-memory copy primitive. Then run guest_output_pointer_fault on macOS or provide a focused result showing both invalid source addresses return EFAULT without terminating the process. The current Windows and WSL2 results do not cover this host-specific branch.
|
Update from the queue: #10978 now also implements these same time exports and adds a checked macOS |
|
Closed my duplicate #10978 in favour of this one. The one addition worth carrying over is the macOS branch of |
|
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.
|
MarcZX10
left a comment
There was a problem hiding this comment.
Requesting one change in the new GuestCopyIn_nid_no_patch helper. On Linux, if process_vm_readv fails with anything other than EFAULT, the helper falls back to std::memcpy(source, bytes). For an invalid source pointer, an EPERM/ENOSYS result (for example, if the syscall is blocked or unavailable) therefore dereferences the untrusted pointer and recreates the host crash this PR is meant to prevent. The existing GuestCopyOut_nid_no_patch treats non-EFAULT syscall failures as infrastructure errors instead of using an unchecked copy. Please use a similarly safe failure path here (or return failure if that is the intended guest-visible behavior) and cover the non-EFAULT syscall failure path.
I reviewed the exact diff and verified that the existing null-output getitimer guard remains in GetitimerInto; this finding is only about the Linux copy-in fallback. The PR's Windows and WSL2 runtime results are author-reported; I did not run the full CMake tests locally.
GuestCopyIn_nid_no_patch fell through to a plain memcpy on macOS, so an unreadable pointer was dereferenced in the host. Read with mach_vm_read_overwrite and require KERN_SUCCESS and a full copy, the mirror of the mach_vm_write branch in GuestCopyOut_nid_no_patch and of CopyGuestBytes. The test now also checks the order FreeBSD 11.4 uses in sys_setitimer (sys/kern/kern_time.c:803-808): the new timer is set first and the old value is copied out afterwards, so a failing copyout reports EFAULT and leaves the timer armed. The macOS branch has not been run. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: AdnanKhan-ak47 <93566182+AdnanKhan-ak47@users.noreply.github.com>
18b725f to
4ab1636
Compare
…EFAULT When process_vm_readv failed with anything but EFAULT (EPERM under a seccomp filter, ENOSYS), GuestCopyIn_nid_no_patch fell back to a plain memcpy, so the host dereferenced the unchecked guest pointer, the crash the checked read exists to prevent. Throw as GuestCopyOut_nid_no_patch does for process_vm_writev. The test runs the call in a child process under a seccomp filter that answers process_vm_readv with EPERM and expects the exception. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks for both points. Head macOS: Linux: when
AI-assisted (Claude Code). |
ahmadmuj
left a comment
There was a problem hiding this comment.
Reviewed head d69520c794feeaaddcd5cbd409dd738fb6ae4504 (base 66e6772ca2094b665488d6ca5cbae3f6fb0c5034). git merge-tree onto current main 4017310678562c63594d99b8f45db2d68b9aaa6c is clean.
What I ran, WSL2 Ubuntu 24.04, GCC 13.3, Release:
- Built only the focused targets.
guest_output_pointer_fault,guest_fstat_fault,guest_freebsd_clocks,guest_getitimer,guest_itimer,guest_itimer_pending_default,guest_system_configuration,guest_time_extras,guest_kernel_sys_ktimerandpublic_headerspass 10/10. The four timer/fault tests also pass 20/20 with--repeat until-fail:20.check_conventions.pypasses. - Red check 1:
Signals.cppandTime.cppback to base, test kept.guest_output_pointer_faultdies with SIGSEGV. - Red check 2:
General.cppfrom 4ab1636 (thememcpyfallback on a non-EFAULTprocess_vm_readverror). The seccomp child fails at line 59, so that test does catch the EPERM case. - Compared against FreeBSD
releng/11.4sys/kern/kern_time.c.gettimeofdaycopiestpfirst, thentzponly if that worked (L644-652).getitimercheckswhichbefore the copyout (L754, L745).setitimerwithitv == NULLforwards to getitimer (L797-800), and copiesoitvout after the timer is armed (L805-808). Your code matches all of that, and the test pins the armed-then-EFAULT case.
One suggestion:
GuestCopyIn_nid_no_patch re-implements CopyGuestBytes from the same file (General.cpp:86-104), which is already a checked read on every host. The copies differ in two places. In #else, GuestCopyIn does a raw memcpy and returns true, while CopyGuestBytes and GuestCopyOut both throw there (CONTRIBUTING: does what it should or throws). On Windows it does VirtualQuery then memcpy, while CopyGuestBytes uses ReadProcessMemory, which can't race with an unmap between the check and the copy. Keeping your null/overflow guard and delegating works:
extern "C" bool GuestCopyIn_nid_no_patch(void* destination, const void* source, std::size_t bytes) {
if (bytes == 0) return true;
const auto start = reinterpret_cast<std::uintptr_t>(source);
if (start == 0 || bytes > UINTPTR_MAX - start) return false;
return CopyGuestBytes(destination, source, bytes);
}I tried that locally: same 4 timer/fault tests pass 5/5, including the seccomp child (CopyGuestBytes throws std::runtime_error on non-EFAULT too). It also drops about 28 lines.
Small notes, not blocking:
setitimer(3, (GuestItimerval*)1, &ok)gives EINVAL here. On FreeBSD the copyin (L803) runs before thewhichcheck inkern_setitimer(L822), so it gives EFAULT. That order is old code, and you said you kept it, so this is just a note. It's now reachable because the bad-pointer path exists.- No test hits the
which != 0copyout ofprevious(Signals.cpp:442-445), e.g.setitimer(1, zero, bad)should be EFAULT. - GuestOutputPointerFault.cpp:75-77 sets
*__error_nid_postfix() = 0three times in a row.
Limits: Linux only, no Windows or macOS run, no dump.
MarcZX10
left a comment
There was a problem hiding this comment.
I reviewed the exact head d69520c794feeaaddcd5cbd409dd738fb6ae4504 and the focused test report. The Linux non-EFAULT fallback and macOS unchecked read issues are addressed, and the added seccomp regression covers the former.
One remaining correctness issue is the final #else in GuestCopyIn_nid_no_patch: it uses std::memcpy and returns success on every host without a platform-specific checked-copy path. An invalid guest source can still terminate the host there. The repository already has the cross-platform checked CopyGuestBytes helper; please keep the zero/null/overflow guard and use that helper (or make unsupported hosts fail safely) instead of falling through to raw memcpy.
Please also add the focused setitimer previous-value copyout fault case noted in review (valid value, invalid previous) if feasible, since it exercises the GuestCopyOut branch after the timer update. Then rerun the focused test and report which host(s) ran it. I have not run the full CMake suite locally.
The last branch of GuestCopyIn_nid_no_patch was a plain memcpy that reported success, so an unreadable pointer on any other host ended the process. Keep the null, overflow and zero-length guards and make exactly one call to CopyGuestBytes, which reads with ReadProcessMemory, process_vm_readv or mach_vm_read_overwrite and throws on a host without a checked read. The per-platform branches are removed. On Windows this replaces the VirtualQuery walk and memcpy with ReadProcessMemory. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
MarcZX10
left a comment
There was a problem hiding this comment.
Thanks for the macOS checked-copy addition and the Linux seccomp regression; the reviewed current diff now has safe host-copy implementations for those cases. One part of the reported issue still appears uncovered: the getitimer and setitimer bodies do not change in this PR, and Time.cpp only routes gettimeofday/sceKernelGettimeofday and sceKernelGettimezone through GuestCopyOut. Could you confirm how the bad getitimer output and setitimer value/previous pointers are made safe on this head, or add the checked copies and regression cases if they remain direct dereferences? The issue specifically lists those pointers; I do not see them covered by guest_output_pointer_fault in the changed test diff.
AdnanKhan-ak47
left a comment
There was a problem hiding this comment.
Reviewed head d9e8185cc1 on macOS 27.0.1 (Apple silicon), the host nobody else in the thread has.
The macOS run everyone asked for is not possible on main. core/libs does not build on macOS at all: GuestAllocations.cpp:26 includes the Linux-only <link.h> under a guard that does not exclude Apple, and docs/dev/BUILD.md:17 states the libraries ship for Linux and Windows only. So guest_output_pointer_fault cannot run here for this or any PR until the libraries are ported; that is out of scope for this fix and should not hold it.
What can be verified is the macOS branch itself. GuestCopyIn_nid_no_patch now delegates to CopyGuestBytes, whose Apple path is mach_vm_read_overwrite(..., &copied) == KERN_SUCCESS && copied == bytes (General.cpp:98-100), the same code as GuestCopyOut's mach_vm_write counterpart. A standalone x86_64 binary with that exact function, run under Rosetta, on the issue's two addresses plus null and a valid one:
source 0x0000000000000001 -> rejected, destination untouched
source 0xfffffffffffff000 -> rejected, destination untouched
source 0x0000000000000000 -> rejected, destination untouched
source 0x0000000308b44ba8 -> copied, destination holds 12/34
No fault, no partial write. The surrounding setitimer logic is host-independent, so this is the host-specific part of the behaviour that the Linux and Windows runs do not cover.
On the 13:36 request: on this head the bodies do change. getitimer_nid_postfix fills a local through GetitimerInto and copies out with GuestCopyOut (Signals.cpp), setitimer reads value through GuestCopyIn and copies previous out through GuestCopyOut on both the which != 0 and ITIMER_REAL branches, and Time.cpp routes the three time getters the same way. ahmadmuj's red check 1 (bodies reverted, test kept, SIGSEGV) covers that these are the lines that matter.
Approving. The behaviour and ordering match what I had in the closed #10978, and delegating to CopyGuestBytes is the better shape; the two small gaps ahmadmuj noted (setitimer(1, zero, bad) for the which != 0 copy-out, and the copyin-before-which order on FreeBSD) are in AdnanKhan-ak47:fix/libkernel-time-efault (GuestItimer.cpp) if you want to lift them, but neither should block this.
… with an unusable pointer getitimer and setitimer(which != ITIMER_REAL, previous) copy the result out through the checked write in Signals.cpp; the fault test only exercised ITIMER_REAL. Add getitimer(ITIMER_VIRTUAL) and setitimer(ITIMER_VIRTUAL, zero, previous) with an unmapped pointer expecting EFAULT, and the matching calls with a valid pointer. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Thanks again. Head
Hosts: Windows 10 (MinGW-w64 GCC 15.2, Release), this head: ninja exit 0; AI-assisted (Claude Code). |
MarcZX10
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c67997b. The new test cases cover getitimer(1, bad) and setitimer(1, zero, bad), including successful calls with valid pointers, closing the previous non-ITIMER_REAL coverage gap. The checked guest read delegates to CopyGuestBytes; the Linux non-EFAULT syscall failure no longer falls through to raw memcpy, and the new seccomp case covers that failure.
Exact-head local checks: git diff --check, tools/check_conventions.py, and a C++20 syntax check of GuestOutputPointerFault.cpp pass. The PR reports the focused tests passing on Windows and WSL2 Linux; hosted product jobs are skipped, and I did not run the linked suite locally. The macOS library test cannot run because core/libs is not currently buildable on macOS; the standalone checked-read result on macOS is author-reported by another reviewer. With those limits stated, the requested correctness and test changes are addressed.
ahmadmuj
left a comment
There was a problem hiding this comment.
Follow-up on my earlier review. Re-checked head c67997b99bf14ef79e702457f87f732ba62d3e7d (base 66e6772ca2094b665488d6ca5cbae3f6fb0c5034); git merge-tree onto current main d2b3ee3bcc8b3724ab953ea7d1b9419e07a38346 is clean.
What I ran, WSL2 Ubuntu 24.04, GCC 13.3, Release:
GuestCopyIn_nid_no_patchnow keeps the null/overflow guard and returnsCopyGuestBytes(...). That's byte-for-byte the version I tried locally last time.- The same 10 focused tests (
guest_output_pointer_fault,guest_fstat_fault,guest_freebsd_clocks,guest_getitimer,guest_itimer,guest_itimer_pending_default,guest_system_configuration,guest_time_extras,guest_kernel_sys_ktimer,public_headers) with--repeat until-fail:20: 200/200 pass, seccomp child included.check_conventions.pypasses. - Red checks on
guest_output_pointer_fault, one mutant at a time:setitimerwhich != 0branch back to*previous = {}: SIGSEGV, so the newsetitimer(1, zero, bad)line catches it.getitimerwriting*output = {}directly forwhich1/2: SIGSEGV, so the newgetitimer(1, bad)line catches it.GuestCopyInback to a rawmemcpy: SIGSEGV onsetitimer(0, bad, ok).
Approving. The setitimer(3, bad) EINVAL-vs-EFAULT order from my last review is still only a note.
Limits: Linux only, no Windows or macOS run, no dump.
|
Draft 11231 ( |
|
Draft #11264 ( |
|
Draft #11273 (signal calls EFAULT) touches neighbouring lines and adds the same GuestCopyIn_nid_no_patch lines. Proposal: yours first, mine rebases. |
|
Draft #11281 (getsockname/getpeername EFAULT) adds the same GuestCopyIn_nid_no_patch lines. Proposal: yours first, mine rebases. |
The only conflict is in Time.cpp with the guest timezone store of the settimeofday change: gettimeofday reads the timezone under the mutex into a local copy, releases it, then copies it out with the checked write. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Head Measured on that rebase (same tree, base AI-assisted (Claude Code). |
|
Agreed that this one should land first; AI-assisted (Claude Code). |
AdnanKhan-ak47
left a comment
There was a problem hiding this comment.
Re-reviewed 080c77547c. git merge-tree of main 49d1f9b07e with the previously approved d9e8185cc1 produces a tree byte-identical to this head, so the merge commit adds nothing beyond resolving #9682's timezone store: gettimeofday fills current and zone as before and copies each out with GuestCopyOut, clock_gettime still maps the errno 14 path to SceKernelErrorEfault, and settimeofday's tz write goes through the checked copy. Against 49d1f9b07e the PR is the same 6 files (+163/-10) as before. Approval stands.
|
Windows re-run of head #11264, #11281, #11273 and #11231 all reuse this PR's AI-assisted: yes (Claude Code); results above were run on this machine. |
|
I rechecked the new merge head The branch has since fallen behind current |
MarcZX10
left a comment
There was a problem hiding this comment.
Rechecked merge head 080c77547cf1; the reviewed checked-copy implementation remains sound and the Windows/WSL results are useful. Current upstream main has advanced: this branch is 6 ahead /225 behind 14c99b52ac, with main-side changes in General.cpp, libkernel CMake and Signals.cpp since its merge base. The reported focused run is against older 14eb2cda22d2. Please refresh onto current main and rerun the focused time/pointer tests on the resulting exact head before merge. This is a current-head integration/validation request, not a reopened code finding.
|
Merged AI-assisted (Claude Code). |
The only conflict is sceKernelGettimezone in Time.cpp: main now reports the host zone, so the zone is built from that into a local and copied out with the checked write.
|
Merged Retested on Windows 10 (MinGW-w64 GCC 15.2), Release, ninja exit code 0: the 11 focused time, timer, signal and pointer-fault tests pass on the exact head, 30 of 30 tests passed on the first merge head Linux not rerun on this head, WSL2 VM would not start; on the first merge head Not tested: macOS. AI-assisted (Claude Code). |
|
macOS run of head Tree:
So the bad-pointer cases return AI-assisted: yes |
|
Windows run of the current head
AI-assisted: yes (Claude Code) |
What
The six functions compute into a local and copy out with the checked write from #9618 (
GuestCopyOut_nid_no_patch);setitimerreads its value with a new checked read,GuestCopyIn_nid_no_patch. They fail with EFAULT (guest 14, or0x8002000Efrom the sceKernel forms). The order of the existing error checks is unchanged.sceKernelGettimeofdaynow returns0x8002000Einstead of -1 when the copy-out fails.Fixes: #10969
Tested
New
guest_output_pointer_fault: bad pointers1and0xfffffffffffff000throughgettimeofday(both pointers),getitimer,setitimer(value and previous),sceKernelGettimeofdayandsceKernelGettimezoneexpect -1/EFAULT and0x8002000E; a valid pointer still succeeds. Headc8430e739conmain66e6772ca2;git merge-treeagainst currentmaina5a780eaaais clean:guest_output_pointer_fault,guest_fstat_fault,guest_freebsd_clocks,guest_getitimer,guest_itimer,guest_itimer_pending_default,guest_system_configurationand thepublic_headersaudit. WithGeneral.hpp,General.cpp,Signals.cppandTime.cpptaken frommainand the test kept,guest_output_pointer_faultfails with a SegFault.main,guest_output_pointer_faultfails with a SegFault.check_conventions --base upstream/mainpasses.main801d5b9bc1plus the open macOS build PRs, this PR's diff on top, three runs per row;GuestCopyIn_nid_no_patchandGuestCopyOut_nid_no_patchgo throughmach_vm_read_overwriteandmach_vm_writethere):guest_output_pointer_faultpasses with this PR and crashes with a segmentation fault whenGeneral.hpp,General.cpp,Signals.cppandTime.cppare restored from the base;guest_getitimer,guest_itimer,guest_itimer_pending_default,guest_time_conversionandguest_time_extraspass either way, on head78cb208c0c(comment).BUILD_TESTING=ON) on the current head78cb208c0cmerged ontomainc547deaacb:guest_output_pointer_fault_testsbuilds without errors andguest_output_pointer_faultpasses both runs ofctest --repeat until-fail:2; withmain'sGeneral.hpp,General.cpp,Signals.cppandTime.cppand the rest of this head it crashes with0xc0000005. The other tests of this description's set, macOS and a title were not run there (comment).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)AI-assisted (Claude Code).
🤖 Generated with Claude Code