Repository navigation
Conversation
The select route's fd_set poison dup2s the route timerfd over every fd below PSELECT_ROUTE_NFDS whose bit is set in the stamp and restores only stdio, so a pseudo-random set of low fds is silently destroyed at every route prepare(). The victim's parent-held pipe ends (task_read, cmd_write, uid_read) landed in that range: on diting 5.10 the W2 child saw EOF at the dance's prepare(), ran its post-loop `getuid() != 0` guard, and _exit(1)'d ~5 ms before the route's walk; verify_w2_stage then hit EPIPE (silently, no print) on every attempt, so the cred write never had a live task to land on and W2 retried forever. Move the parent-held ends above the route's fd range with F_DUPFD_CLOEXEC; open_selected_fds never touches fds >= PSELECT_ROUTE_NFDS. Co-Authored-By: Claude Code <noreply@anthropic.com>
…s clobbers open_selected_fds rebinds every fd < PSELECT_ROUTE_NFDS whose bit appears in the poisoned fd_sets (the waiter stamp) to the route timerfd. The bits are arbitrary address bits, so the touched set is quasi-random per boot, and only stdio had a backup; destroy() then closed the entire set. Net effect: the route silently replaced (and finally closed) unrelated long-lived app fds at every dance. Proven on diting 5.10: the W2 victim's command pipe was hit at prepare(), the child saw EOF and _exit(1)'d mid-dance via its `getuid() != 0` guard, and the post-route verify could never pass (EPIPE, silently). Snapshot each touched slot (F_DUPFD_CLOEXEC copy + original FD_CLOEXEC state) before the dup2, restore after the pselect returns (next to restore_standard_io), and have destroy() restore instead of close. Originally closed fd numbers are closed again; the consumer-stuck path keeps the installed descriptors, matching retain_for_process_lifetime(). Co-Authored-By: Claude Code <noreply@anthropic.com>
verify_w2_stage() returned 0 silently on both the command write and the reply read, which made two different failures the same log line: none. A child that is dead and a child that is alive but never answers both read as "no output between the round headers" -- and that ambiguity is what a whole series of W2 failures looked like. With errno it is unambiguous. `reply read 0 of 4 bytes (errno=0)` is EOF, i.e. the child is gone; `no command sent (errno=32)` is EPIPE on a pipe whose reader has exited. That is what moved the investigation off the write vehicle and onto what was happening to the victim after it was rooted. Co-Authored-By: Claude Code <noreply@anthropic.com>
…he select fds open_selected_fds() snapshots every slot the stamp's bits hit before the dup2 pass rebinds it to the route's descriptor. A failed F_DUPFD_CLOEXEC left backup = -1 -- the same value a slot that was closed before the dance gets -- and restore_selected_fds() closes the slot for either, so a live descriptor that could not be copied (EMFILE, or EINVAL when the fd limit sits below the copy range) was indistinguishable from one that was never open. Read F_GETFD first, which also replaces the second fcntl on the success path, so the two cases separate without adding syscalls: a closed slot keeps backup = -1, a live slot whose copy failed is counted and reported once with its errno. Both still end closed at restore -- the dup2 in the same iteration has already destroyed the slot's original value, so there is nothing left to put back -- but the loss is now visible instead of silent, and it is exactly the case where the protection this pass exists for did not apply. Not reachable on diting 5.10.236 as measured (RLIMIT_NOFILE 32768, ~149 fds in use, copy range >= 384); no run has hit it. Co-Authored-By: Claude Code <noreply@anthropic.com>
Haoyu-UT
changed the title
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Descriptor 319 and backup failure paths can still destroy unrelated descriptors, and restore ownership lacks required isolation and tests.
5 open findings
What changed in this PR
Protects process descriptors during the select_stack route and improves W2 failure diagnostics.
Changes:
- Moves victim protocol descriptors above the select range.
- Backs up and restores descriptors replaced by the route.
- Logs W2 pipe failures.
| File | Description |
|---|---|
src/core/session/victim_process.cpp |
Protects protocol pipes and adds verifier diagnostics. |
src/core/route/select_stack_route.cpp |
Adds descriptor backup and restoration. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+257
to
+261
| if (e.backup >= 0) { | ||
| if (dup2(e.backup, e.fd) >= 0 && e.cloexec) { | ||
| (void) fcntl(e.fd, F_SETFD, FD_CLOEXEC); | ||
| } | ||
| close(e.backup); |
Comment on lines
282
to
283
| for (int32_t fd = 0; fd < PSELECT_ROUTE_NFDS; fd++) { | ||
| if (FD_ISSET(fd, in) || FD_ISSET(fd, out) || FD_ISSET(fd, ex)) { |
Comment on lines
+251
to
+252
| static std::array<SelectedFdBackupEntry, PSELECT_ROUTE_NFDS> selected_fd_backup; | ||
| static size_t selected_fd_backup_n = 0; |
Comment on lines
+192
to
+194
| move_fd_above_route_range(p.task_read); | ||
| move_fd_above_route_range(p.cmd_write); | ||
| move_fd_above_route_range(p.uid_read); |
| static std::array<SelectedFdBackupEntry, PSELECT_ROUTE_NFDS> selected_fd_backup; | ||
| static size_t selected_fd_backup_n = 0; | ||
|
|
||
| void restore_selected_fds(void) { |
Author
|
Let me fix and re-open |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stop the select route from destroying unrelated file descriptors
Note
This a preliminary fix for 5.10 series kernel support.
Problem
SelectStackRoutebuilds its pselect fd_sets out of the waiter stamp, so the payload words are the bitpattern:
open_selected_fds()dup2s one of the route's own descriptors over every fd <PSELECT_ROUTE_NFDS(320) whose bit happens to be set, anddestroy()closed that quasi-random set. Onlystdio had a backup. Any
long-lived descriptor in that range — app fds, and the W2 victim's protocol pipes — was silently replaced
for the length of the dance and closed afterwards.
Observed while porting to a 5.10 kernel: the victim's command pipe was replaced at
prepare(), the rootedchild read EOF, took its post-loop
getuid() != 0guard and exited about one dance before the cred writelanded.
verify_w2_stage()then hit EPIPE on every attempt and returned 0 without printing anything, soW2 retried forever against an empty log. Every write landed; the chain never completed.
Changes
fix(victim): keep protocol fds out of the select route's fd rangeF_DUPFD_CLOEXEC; the dup2 pass never reaches themfix(route): snapshot and restore every fd the select route's dup2 pass clobbersFD_CLOEXECstate captured — before the dup2, restored once the pselect returns;destroy()restores instead of closingfix(session): report why the W2 verifier gave uperrno, which separates a dead child (EPIPE, 0-byte reply) from a live child that is not consuming (full pipe, EAGAIN)fix(route): tell a closed fd from an uncopyable one when backing up the select fdsbackup = -1;F_GETFDnow separates them, and a lost descriptor is reported instead of silently closedThree details worth knowing while reviewing:
PSELECT_ROUTE_NFDS + 64, above the range the pass walks — a copy inside the rangecould be poisoned by a later iteration of the same loop.
dup2clearsFD_CLOEXECon its target, so the flag is read before the dup2 and re-applied after;otherwise a close-on-exec descriptor would come back without it.
destroy()'s call is a no-op when the per-attempt restore already ran.The consumer-stuck path deliberately keeps the installed descriptors, matching
retain_for_process_lifetime().Testing
Device: Redmi K50 Ultra (
diting, SM8475),5.10.236-android12-9-00003-gfb24cf99ad97-ab14313284,select_stackroute, on both ROM builds that share that kernel (CN and Xiaomi.eu). Before: the chainstopped at W2 on every run, silently. After: W1 → W2 (
child is root!) → W3 → root script atuid=0→KernelSU late-load, across a run series on the patched and on the stock kernel.
Not tested: 6.6 / 6.12, the families where
select_stackis the primary route — we have one 5.10device. The change is descriptor bookkeeping with no kernel-version-dependent code, and the two other
routes never touch the fd table (
tcp_zerocopy_route.cppandmulticast_waiter_route.cppcontain nodup2/F_DUPFD), so the blast radius is the select route's own fd handling. The last commit's failurebranch is unreachable on our device (
RLIMIT_NOFILE32768, ~149 descriptors in use, copies need fd ≥ 384)and is behaviorally identical on the success path — same syscall count.