Repository navigation
fix(libkernel): refuse read-only guest mappings for getdirentries on Windows - #7672
Conversation
|
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 |
jotitadev
left a comment
There was a problem hiding this comment.
Linux run, since the description has a syntax check only and the existing run is Windows.
Machine: Linux x86_64, i7-8700, Ubuntu 26.04, GCC 15.2, Release, BUILD_TESTING=ON, full build.
Head 7709eda9a merged onto main 84c1a4d9a (clean merge).
- Builds. The directory/filesystem tests pass (
ctest -R "director|dirent|getdents|filesystem|kernel_sys", 8 tests), and the non-GPU suite (ctest -E "^agc|gpu|vulkan|video_out") is 739/739, same asmain. - The new check does not fire on Linux. Outside Windows,
GuestArenaBeginHostWrite_nid_postfix(GuestArena.cpp) only notifies the write watch and returnstrue, soHostWrite::Open()is always true and theEFAULTreturn is unreachable. A small probe (each call in a forked child, fresh descriptor per call, on an empty directory) gives the same result with and without this PR:- valid
bufandbasep: returns 24; buf = 0x1000(unmapped, outside the arena): SIGSEGV;basep = 0x1000: SIGSEGV.
- valid
So on Linux this changes nothing, and the "forged pointer fails with EFAULT" part of the description holds only where BeginHostWrite rejects the range. @MarcZX10 raised the same point about HostWrite::Open() for #7675, which uses the same guard.
Written with AI assistance (Claude Code). I ran the builds and tests above on my machine.
|
Tested at The change helps on Windows. With The description says more than the change does, though.
Could the title and description say "refuses read-only guest mappings on Windows", and could the PR add a test for that case? AI-assisted: yes (Claude Code); results above were run on this machine. |
sceKernelGetdirentries/sceKernelGetdents wrote up to nbytes into buf and 8 bytes into *basep with only null checks, unlike sceKernelRead, pread and readv which all gate on GuestArena::HostWrite. Validate both buffers the same way so a forged pointer fails with EFAULT instead of writing to an arbitrary host address.
7709eda to
25d26b5
Compare
|
Thanks for the independent build and test run — that was genuinely useful. You're right that the description claimed more than the change does, so I've narrowed it: the PR is retitled to I've also added the test you asked for: I couldn't execute the test suite in this environment (no Windows toolchain here), so CI will have to run it — your finding that unpatched |
…ite and check the descriptor first
|
Ran
One detail in the test: when the read-only Opening a fresh descriptor, or rewinding, before the read-only AI-assisted: yes (Claude Code); results above were run on this machine. |
|
Updated in 950eeb8:
Built and ran on Windows and Linux; details are in the description.
|
|
@wachi35676 thanks — #7672 merged at The follow-up closes the descriptor after the writable control, opens a fresh one before the read-only All four Windows directory tests pass. With only AI-assisted: yes. |
What
sceKernelGetdirentriesnow guards itsbufandbasepoutputs withGuestArena::HostWritein both the Windows and Linux branches ofcore/libs/prx/libkernel/File/src/Stdio.cpp. This matches the other output-buffer syscalls in that file. Both guards stay open until the call returns. They open after the descriptor checks, so a bad descriptor still fails withEBADForEINVALbeforeEFAULT. FreeBSD 11.4'skern_getdirentriesalso checks the descriptor before reading.What this does: on Windows, a
buforbasepin a read-only guest mapping (for example, a read-only direct-memory mapping) now fails withEFAULT(0x8002000e). Previously the process crashed with an access violation.What this does not do:
HostWriteonly refuses ranges that overlap a non-writable shared view; it does not establish that the pointer is in the guest arena.GuestArenaBeginHostWritealways returnstruethere, so the read-only mapping still crashes on Linux. On Linux the guard only reports the write to the write watch.Tested
Merged at
950eeb847on 2026-10-19. The subsequent fresh-descriptor test improvement is tracked separately in #7892; it was not part of this merge.Each build is this branch merged onto
maina373e85e5, Release,BUILD_TESTING=ON.guest_*tests registered undercore/libs: all pass, the same asmain.guest_video_out_wqhd_detection_no_vulkanis skipped on both.guest_directory_buffer,guest_directory_rewindandguest_directory_close.Stdio.cppfrommain, the original regression test fails at its read-only output check. As @wachi35676 clarified, thebufcall was already at EOF; the distinctbasepprobe is the one that demonstrates the access violation. test(libkernel): exercise read-only getdirentries buffers before EOF #7892 opens a fresh descriptor so thebufcheck also exercises the crash.guest_*tests pass, includingguest_directory_entries,guest_filesystem,guest_kernel_sysandguest_kernel_app_archive_*.guest_system_load_exec_standard_errorandguest_named_shm_concurrency, fail onmainin the same container too; it runs as root.CheckReadOnlyDestinationincore/libs/tests/GuestDirectoryBuffer.cppis part of the Windows-only test binary. It covers:bufsucceeds.EFAULT, both asbufand asbasep.bufreturnsEBADF.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)