Repository navigation
fix(libkernel): fail recvmsg with EFAULT on guest memory it cannot read or write - #7675
Conversation
|
One remaining pointer-validation gap: |
|
More important, |
|
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.
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.
|
|
Windows build and run, since the description has a syntax check only. Radeon 860M (RDNA 3.5, driver 32.0.13058.8001), Windows 11 26300, WinLibs GCC 15.2.0 r7, Release, The build is clean. The full AI-assisted |
|
Thanks for posting the Windows run. The 474/476 result is useful for the composed tree, with the two failures attributed to #7658. As you noted, it does not exercise the new recvmsg validation path, so it does not resolve the concrete guest-pointer issues from my earlier comments: the iovec array is read before validation, and |
a482f91 to
f5ef1c1
Compare
|
All three points addressed in the latest push: 1. Iovec array validated before 2. One deliberate design call: I used committed-range rather than strict guest-arena containment. A 3. Focused tests added in |
|
I think the new |
jotitadev
left a comment
There was a problem hiding this comment.
Linux run of f5ef1c1cb. It breaks guest_udp.
Machine: Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON, full build.
Head f5ef1c1cb merged onto main 84c1a4d9a (clean merge).
-
guest_udpaborts with this head, 3 of 3 runs.mainpasses, and so did the previous heada482f911b. With theRequirecalls numbered, it stops atGuestUdp.cpp:94:recvmsg(receiver, &message, 0)returns -1 with errno 14. That check runs in the loop over name capacities{0, 1, 2, addressSize - 1, addressSize, 32, UINT32_MAX}on a 32-bytestd::array.CommittedRange(message->name, message->nameLength)checks the capacity the caller declares, not the bytesrecvmsgwrites, and an oversized capacity is valid input that the test covers. The iovec lengths are checked the same way. A probe on a Unix datagram pair, sending 4 bytes each time:call mainf5ef1c1cb32-byte name, nameLength = 324 4 32-byte name, nameLength = UINT32_MAX4 -1, errno 14 16-byte iov[0].base,length = 1 GiB4 -1, errno 14 message,iov[0].baseormessage->iov=0x1000(unmapped)SIGSEGV -1, errno 14 message->iovoriov[0].baseon aPROT_NONEpageSIGSEGV SIGSEGV iov[0].baseon aPROT_READpageSIGSEGV SIGSEGV The last two rows match @MarcZX10's point about
mincore. -
The new
guest_socket_paircases pass. With onlyGuestSocketPair.cppfrom this PR,mainsegfaults, so the new cases do cover the crash. -
Non-GPU suite (
ctest -E "^agc|gpu|vulkan|video_out"): 738/739. The one failure isguest_udp;mainis 739/739.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Thanks, both. Reworked in 3cb4ae8. @jotitadev: the
I reproduced your abort with @MarcZX10:
Trade-off: an output fault is found after the host receive, so the data is dropped. FreeBSD also drops a datagram on a copy fault, but keeps unread stream data. The libkernel technical debt file records this, along with the unchecked copies on macOS. Results are in the description. A receive into write-watched memory was not exercised on Linux, because userfaultfd write protection was not available in my environment. |
jotitadev
left a comment
There was a problem hiding this comment.
macOS run of 3cb4ae859. The new CheckReceiveMessageFaults crashes on a macOS host, and XNU has a checked-copy equivalent that makes it pass.
Machine: Apple M2 Max, macOS 27.2, Xcode 27 clang, Release, BUILD_TESTING=ON, x86_64 under Rosetta. Not main: main does not build on macOS yet, so the base is main a373e85e5 + mugurc's ci/macos-series-2 (91e4eccaf) + the macOS bits of #7332. The PR diff applied cleanly on top (git apply -3). I rebuilt and ran the socket tests (ctest -R 'guest_(socket|udp|tcp|sendfile)', 14 tests).
-
guest_udppasses with this head. The regression fromf5ef1c1cbis gone on macOS too. -
guest_socket_pairaborts on macOS both with and without this PR, at an existing check:shutdown(SHUT_RD)on the peer aftershutdown(SHUT_WR)returnsENOTCONNon XNU. That abort comes beforeCheckReceiveMessageFaults, so the full test never reaches the new cases. I ran them on their own with a local test-only patch that callsCheckReceiveMessageFaults()first:- base + this PR's test only: SIGSEGV (expected);
- this PR: SIGSEGV on the first case, the header at
0x1000. On macOSReadGuestandWriteGuestfall through to plainmemcpy. The debt entry says the copies are unchecked on macOS, but the test is not gated, so it crashes on a macOS host.
-
XNU primitives, standalone probe (arm64 native and x86_64 Rosetta give the same results).
mach_vm_read_overwriteandmach_vm_writeonmach_task_self()behave likeprocess_vm_readv/process_vm_writev:target mach_vm_read_overwritemach_vm_writeRW page OK OK PROT_READpageOK KERN_INVALID_ADDRESS, page unchangedPROT_NONEpageKERN_INVALID_ADDRESSKERN_INVALID_ADDRESS0x1000KERN_INVALID_ADDRESSKERN_INVALID_ADDRESSrange spanning RW into PROT_NONEKERN_INVALID_ADDRESSKERN_INVALID_ADDRESS -
With an
__APPLE__branch that uses those two calls (below), every case inCheckReceiveMessageFaultspasses on macOS. The other socket tests show the same results as base: 13/14, with only the pre-existingguest_socket_pairabort.
#elif defined(__APPLE__) // ReadGuest (with <mach/mach.h>, <mach/mach_vm.h>)
mach_vm_size_t copied = 0;
return ::mach_vm_read_overwrite(::mach_task_self(), reinterpret_cast<mach_vm_address_t>(source), bytes,
reinterpret_cast<mach_vm_address_t>(destination), &copied) == KERN_SUCCESS && copied == bytes;
#elif defined(__APPLE__) // WriteGuest
return ::mach_vm_write(::mach_task_self(), reinterpret_cast<mach_vm_address_t>(destination),
reinterpret_cast<vm_offset_t>(source), static_cast<mach_msg_type_number_t>(bytes)) == KERN_SUCCESS;So there are two options: use this branch and drop "unchecked on macOS" from the debt entry, or gate CheckReceiveMessageFaults off on __APPLE__. I did not test a receive into write-watched memory on macOS, and I did not test an arm64 build of the tree.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
jotitadev
left a comment
There was a problem hiding this comment.
Linux rerun of 3cb4ae859 with the same probe as my report on f5ef1c1cb.
Machine: Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON, full build. Head 3cb4ae859 merged onto main 8816fd772 (clean merge).
-
guest_udppasses again, 3 of 3 runs directly and under ctest. The socket tests pass (12/12) and the non-GPU suite is 775/775, same asmain. -
The PR's
guest_socket_paironmain(library unchanged) segfaults, so the new cases cover the change. -
Probe (
recvmsgon a socketpair with a 4-byte message):case f5ef1c1cb3cb4ae859message,message->iovoriov[0].baseat0x1000EFAULTEFAULTmessage->iovon aPROT_NONEpageSIGSEGV EFAULTiov[0].baseon aPROT_NONEpageSIGSEGV EFAULTiov[0].baseon aPROT_READpageSIGSEGV EFAULT32-byte name, nameLength = UINT32_MAXEFAULT4, as on main16-byte iov, length 1 GiB EFAULT4, as on main
So on Linux both the regression and the mapped-but-inaccessible cases are fixed. The macOS point from my other comment (plain memcpy there) is unchanged by this head.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
3cb4ae8 to
cec28a2
Compare
|
@jotitadev thanks for the macOS probe and Linux rerun. Addressed in f7fd9db, rebased onto
Locally: 19/19 focused socket tests pass on Windows (GCC 15.2 r7), and 5/5 on Linux (GCC 14.2). I cannot run macOS here, so I am not claiming a macOS build or execution result. Could you rerun AI-assisted: yes. |
recvmsg_nid_postfix wrote the msghdr, every iovec base and the sender address with only null checks, unlike recv_nid_postfix and the sceNetRecvfrom sender outputs which all gate on GuestArena::HostWrite. Validate the message, each iovec base and the name buffer the same way so a forged pointer fails with EFAULT instead of writing received bytes to an arbitrary host address.
Address review: check the msghdr, iovec array, iovec bases and name are mapped before MessageLength or the receive loop touches them, pairing the HostWrite guard with committed-range validation. Add regression tests for forged iovec-array and output pointers.
cec28a2 to
f7fd9db
Compare
What
recvmsg_nid_postfixread themsghdrand the iovec array, and wrote the received data, the sender address and themsghdrresults, with only null checks. This PR makes those copies fail withEFAULTinstead of faulting. It checks only the bytes that are actually read or written.msghdrand the iovec array are copied in before use. On Linux this usesprocess_vm_readvon the process itself. On Windows it usesVirtualQuery, requiring committed, readable pages. On macOS it usesmach_vm_read_overwriteon the current task. If the header or iovec array is unmapped, or on aPROT_NONE/PAGE_NOACCESSpage, the call fails withEFAULTbefore anything is received.nameLength,controlLengthandflagsare written underGuestArena::HostWrite, covering exactly the bytes written. On Linux this usesprocess_vm_writev; on macOS it usesmach_vm_writeon the current task. On Windows,VirtualQuerymust report a writable page after the guard opens, so an armed write-watched view is still accepted. A read-only or unmapped destination fails withEFAULT.guest_udpregression in the previous head (nameLength = UINT32_MAXon a 32-byte buffer, or a 1 GiB iovec over a 16-byte buffer).soreceive_dgram,soreceive_generic) but keeps unread stream data. The libkernel technical debt file records this and that a Linuxprocess_vm_*error other thanEFAULTthrows. The unchecked macOS-copy gap has been removed.This replaces the previous
CommittedRangecheck (mincore/MEM_COMMIT), which caused theguest_udpregression and did not catch inaccessible mapped pages.This overlaps #7090 (SCM_RIGHTS), which also changes
recvmsg_nid_postfix. Whichever PR merges second will need a rebase.Tested
Latest follow-up: rebased onto
mainb106f6f8c(including merged #7672), Release,BUILD_TESTING=ON.guest_recvmsg_memory,guest_socket_pair,guest_udp,guest_tcp,guest_socket_timeoutsand the 14guest_socket_host_write_*tests.guest_recvmsg_memory,guest_socket_pair,guest_udp,guest_tcp,guest_socket_timeouts.guest_recvmsg_memoryCTest entry runs the memory-fault cases independently (guest_socket_pair_tests recvmsg-memory). It can run even when an earlier, unrelated socketpair check fails on macOS.mainbaseline on Windows and Linux. The earlier fail-on-main check with this PR's tests and unmodifiedUdp.cppsegfaults on both platforms.a373e85e5: the Windows guest suite passed (one Vulkan-absence test skipped); Linux had only two failures also present on its baseline. Those broad suites were not rerun for this follow-up.CheckReceiveMessageFaultsincore/libs/tests/GuestSocketPair.cppruns on both platforms. It uses flexible memory with a writable, a read-only and a no-access page:EFAULT, datagram still queued: header at0x1000, header on the no-access page, iovec array at0x1000, iovec array on the no-access page.EFAULTafter the receive: iovec base at0x1000, iovec base on the no-access page, iovec base on the read-only page, name at0x1000, name on the read-only page, header on the read-only page. Nothing is left queued afterwards.nameLength = UINT32_MAX: 16 bytes are written and the rest of the buffer is untouched.Not run locally: macOS. The Mach implementation follows @jotitadev's independently run macOS probe/review; a rerun of this exact head is requested. No local macOS build or write-watch result is claimed.
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)