Skip to content

fix: prove installed module bytes during updates - #13

Merged
exploitintel merged 9 commits into
mainfrom
fix/update-path-module-verification
Sep 15, 2026
Merged

exploitintel merged 9 commits into
mainfrom
fix/update-path-module-verification

Conversation

@exploitintel

Copy link
Copy Markdown
Owner

Ports the Pixel 11's merged PR 29 to the Pixel 8a, adapted to this repo's update flow (version-string gated park + ksud + reboot, which already existed here):

  • Installed module bytes are now proven after every update: the payload tree is re-staged through a mode-preserving overlay and verified on the phone against a payload-built checksum manifest with toybox-compatible flags, pruning files the payload no longer contains. Runs in the installer's Docker-stopped windows (post-reboot and post-park).
  • Module reinstalls fetch and hash-verify the pinned Docker Engine archive when neither the bundle nor the phone holds it; the pin now ships beside install.sh in the bundle.
  • The rationale is the observed KernelSU behavior (fresh module.prop beside stale bin/hostctl on a Pixel 11, 2026-09-15).

Independent review completed (ready to push); its three findings are fixed in this commit: a vacuous no-restart assertion now checks the 8a's actual restart path, the phone-held engine-archive branch gained a test, and the Docker-stopped wording no longer overstates the boot-autostart edge. Tests: 34/34 in the installer suite plus the full tools/check.sh gate.

Comment thread deployment/simple-install.sh Outdated
engine_url=$(sed -n 's/.*"url": "\([^"]*\)".*/\1/p' "$engine_json" | head -n 1)
engine_name=${engine_url##*/}
fi
if [[ -n "$engine_name" ]] && phone "test -s /data/local/tmp/$engine_name" >/dev/null 2>&1; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ensure_engine_archive trusts a pre-existing /data/local/tmp/$engine_name on the phone purely on test -s (non-empty), with no size or SHA-256 check — unlike the download branch three lines below, which verifies both before push. A leftover file from an interrupted prior run (e.g. a partial adb push or a killed installer between download and module install) would satisfy test -s and be reused unverified, skipping this function's own integrity gate entirely.

In practice this doesn't reach a corrupted engine: module/bin/prepare-engine independently re-checks the sideloaded archive's exact size+SHA-256 (require_identity "$SIDELOAD_FILE" "$TARBALL_SIZE" "$TARBALL_HASH", prepare-engine:237) before ksud module install will use it, so a bad reused archive fails that step with 'sideloaded engine archive has an unsafe type or wrong identity' rather than being installed. But that means install.sh is currently asserting a guarantee ("verified" identity) for a file it never actually verified, and the failure — if it ever triggers — surfaces as a cryptic module-install error deep in ksud output instead of this script's own clear diagnostic.

Suggest verifying size+hash on the existing file too (reuse verify_file) before returning early, same as the download path does.

Comment thread deployment/simple-install.sh Outdated
Comment on lines +208 to +214
# KernelSU's module update can preserve previously installed file bytes
# (observed on the Pixel 11 Pro XL on 2026-09-15: a new module.prop beside a
# stale bin/hostctl), so installed module bytes are always re-staged from the
# payload through a mode-preserving overlay and proven on the phone against a
# payload-built checksum manifest, with files the payload no longer contains
# pruned. Runs in the installer's Docker-stopped windows; boot-time
# autostart can still race on the post-reboot site.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment (and the matching README.md:155 claim "the installer's Docker lifecycle stopped") isn't accurate for the call site at line 548 (existing-install, module-already-current path).

Trace: start_docker() is called unconditionally at line 530 — before wait_until_parked (546) and verify_module_files (548) — and start_docker actually launches dockerd (/data/docker/bin/hostctl start, which spawns dockerd.sh/dockerd). wait_until_parked only parks Forge's own containers via eip-hostctl.sh park-when-idle; the underlying eip.sh down call in eip-hostctl.sh explicitly documents itself with die 'Forge did not stop cleanly; Docker remains running' — i.e. parking is defined to leave the Docker daemon running. So by the time verify_module_files runs in this branch, dockerd has been running for several stages already, and only gets "restarted" afterward at line 550-551.

This happens to be harmless today only because verify_module_files touches /data/adb/modules/eip-pixel8a-forge (the KernelSU module tree), while the live dockerd/hostctl run from the separately-managed /data/docker/bin release tree populated by release-transaction — so the two don't collide. But the comment states a safety invariant ("Docker-stopped windows") that the code doesn't actually provide at this call site, and nothing enforces or tests it. A future change to either directory layout could silently turn this into a real race with no test to catch it. Worth either correcting the comment to describe the real guarantee (directory separation, not "Docker stopped"), or actually stopping dockerd here if that was the intended invariant.

KernelSU's module update can preserve previously installed file bytes
beside a fresh module.prop, and the update path never proved what ksud
left on disk. Updates now re-stage the payload module tree through a
mode-preserving overlay while Docker is stopped, verify it on the phone
against a payload-built checksum manifest with toybox-compatible flags,
and prune files the payload no longer contains. Module reinstalls also
fetch and hash-verify the pinned Docker Engine archive when neither the
bundle nor the phone holds it. Ports the Pixel 11 fix (its PR 29).
@exploitintel
exploitintel force-pushed the fix/update-path-module-verification branch from d123a84 to d100809 Compare September 15, 2026 11:35
Comment thread deployment/simple-install.sh Outdated
push "$work/files.tar" /data/local/tmp/eip-module-files.tar
push "$work/manifest" /data/local/tmp/eip-module-manifest
push "$work/names" /data/local/tmp/eip-module-names
phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do rm -f "$stale"; done; rc=$?; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit $rc' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_module_files runs sha256sum -c /data/local/tmp/eip-module-manifest -s directly on the phone through a bare su -c shell — no path to a specific binary. Every other on-device hash operation in this repo (module/uninstall.sh, module/bin/install-preflight, module/action.sh, …) deliberately invokes $BUSYBOX sha256sum (/data/adb/ksu/bin/busybox) rather than trusting whatever sha256sum PATH resolves to under su -c, and none of those existing call sites ever use -c (check mode) — they only compute a single hash and compare in shell.

On this Android 17 (CP2A.260805.005) target, a bare sha256sum in the su -c shell resolves to the system toybox applet. Nothing in this codebase — including the new test coverage in tests/simple-installer.test.mjs, whose fake-tool shim runs the host's GNU/Perl sha256sum/shasum, never toybox — exercises -c/-s against the actual on-device binary. If toybox's sha256sum here doesn't support -c combined with a trailing -s (GNU's equivalent is the long-only --status; there's no short -s), this command fails unconditionally regardless of whether the module bytes are correct.

Since this same call now runs on every existing-install update (unconditionally once wait_until_parked returns, line ~552) and every fresh/version-bump install (line ~527), an incompatible flag here would hard-block every install and update path — worth confirming against the real on-device toybox, or switching to the bundled busybox (/data/adb/ksu/bin/busybox sha256sum -c … -s) to match the rest of the codebase's convention, before this ships.

Comment thread deployment/simple-install.sh Outdated
push "$work/files.tar" /data/local/tmp/eip-module-files.tar
push "$work/manifest" /data/local/tmp/eip-module-manifest
push "$work/names" /data/local/tmp/eip-module-names
phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do rm -f "$stale"; done; rc=$?; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit $rc' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The stale-file prune in this same command (find . -type f | … | comm -23 - eip-module-names | while … rm -f) walks the entire /data/adb/modules/eip-pixel8a-forge tree and deletes anything not present in host-module.zip's own file list.

KernelSU-Next (following the Magisk module convention it's modeled on) tracks per-module user state — e.g. the disable marker a user creates by toggling the module off in the KernelSU-Next Manager UI — as loose files dropped directly in the module directory, outside the shipped module zip. Since this prune now runs unconditionally on every existing-install update (line ~552), not just on module-version bumps, a user who disabled this module through the Manager app would have that disable file silently deleted and the module re-enabled the next time Forge updates, without their consent — a KernelSU module-lifecycle regression, not just a cosmetic one.

Worth confirming KernelSU-Next's actual module-state file conventions here and excluding anything KernelSU/ksud itself owns (disable/remove/update-style markers, if applicable) from the prune, rather than treating "not in the payload zip" as equivalent to "safe to delete."

Comment thread deployment/simple-install.sh Outdated
Comment on lines +185 to +188
engine_url=$(sed -n 's/.*"url": "\([^"]*\)".*/\1/p' "$engine_json" | head -n 1)
engine_name=${engine_url##*/}
engine_size=$(sed -n 's/.*"size": \([0-9][0-9]*\).*/\1/p' "$engine_json" | head -n 1)
engine_sha=$(sed -n 's/.*"sha256": "\([0-9a-f]\{64\}\)".*/\1/p' "$engine_json" | head -n 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packaging: Docker Engine identity isn't parsed from a single source of truth, and the phone-side filename is separately hardcoded.

ensure_engine_archive() pulls url/size/sha256 out of tools/engine.json with unscoped sed patterns and head -n 1. That only picks the right value today because engine.tarball happens to appear before the binaries.* block, which also has 8 more "size"/"sha256" keys (tools/engine.json:48-97). Reordering the JSON, or adding a new object with those keys earlier in the file, would silently pin engine_size/engine_sha to the wrong value — no parse error, just a confusing "wrong size"/"wrong SHA-256" die on the next run.

Separately, lines 508 and 524 hardcode the phone-side filename as the literal docker-29.8.0.tgz instead of deriving it the way engine_name does here (line 186). The module's own install contract ties the expected sideload filename to the engine version: install-preflight rejects an ENGINE row whose filename isn't exactly "docker-" version ".tgz", and that row is regenerated from tools/engine.json at packaging time. If the pinned Docker Engine version in tools/engine.json is ever bumped without touching these two literals, a freshly packaged module would expect docker-<new-version>.tgz on the phone, but this script would still push (508) and clean up (524) docker-29.8.0.tgz — the phone-side kernelctl install would fail to find its expected archive, and the stale-named cleanup would leave the actually-pushed (old-named) archive behind on /data/local/tmp.

Worth scoping the sed to the engine.tarball object and reusing a single derived engine_name for the push/cleanup paths, so a Docker Engine version bump can't silently desync this script from tools/engine.json.

Comment thread deployment/simple-install.sh Outdated
# runs from the separately managed /data/docker release tree.
verify_module_files() {
local work entry
stage 'Verifying the Pixel module bytes' 'Check the module verification output above; Docker stays stopped and existing host state is preserved.'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verify_module_files()'s stage message claims "Docker stays stopped and existing host state is preserved," but that's only true for the first call site (line 541, right after wait_android post-reboot, before start_docker has ever run this session).

The second call site (line 566, inside the EXISTING_INSTALL == true branch) runs after start_docker has already been called unconditionally at line 548 — Docker is never stopped before this point. The README addition in this same PR confirms this is intentional: "the Docker daemon runs from the separately managed /data/docker release tree" during this verification. So if the phone-side tar -xf ... && sha256sum -c ... check fails at the second call site, the operator sees NEXT_ACTION guidance ("Docker stays stopped...") that misrepresents actual system state — Docker (and possibly still-draining Forge work) is running, not stopped. That could lead someone troubleshooting the failure to make an unsafe assumption (e.g., that it's safe to power-cycle or intervene without stopping Docker first).

Since stage()'s message is shared by both call sites inside the single verify_module_files function, consider passing a call-site-specific message, or wording it to not claim Docker's state.

"$ADB_BIN" -s "$SERIAL" reboot >/dev/null 2>&1 || true
wait_android
verify_module_files
elif [[ "$EXISTING_INSTALL" == false ]]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This elif [[ "$EXISTING_INSTALL" == false ]] branch (module already reports the target version, but the rest of the install — Docker disk/state/control app — isn't in place yet, e.g. a previous run got the module installed+rebooted and then failed/was interrupted before Docker setup completed) never calls verify_module_files. That is exactly the failure mode this PR exists to close: KernelSU can leave a fresh module.prop beside stale bin/hostctl/other module files, and boot-time module code (Wi-Fi routing, Docker startup) would run from that unproven tree here.

Contrast with the sibling branches that do verify:

  • line 541, right after the module is freshly installed+rebooted in this same if/elif.
  • lines 565–567, the existing-install park path.

This isn't just a theoretical gap — EXISTING_INSTALL="0" + HOST_MODULE_CURRENT="1" is the default test fixture combination (tests/simple-installer.test.mjs:422-423), so most of the existing suite already exercises this exact branch, and none of the new module-verification tests cover it. The README addition also states "Every update then proves the installed module tree against the payload," which isn't true for this path.

local engine_json=$SCRIPT_DIR/engine.json url
[[ -f "$engine_json" ]] || engine_json=$SCRIPT_DIR/../tools/engine.json
url=$(engine_tarball_block "$engine_json" | sed -n 's/.*"url": "\([^"]*\)".*/\1/p')
printf '%s' "${url##*/}"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor robustness gap: engine_archive_name() has no guard for a missing/malformed engine.json, unlike ensure_engine_archive() just below it which explicitly does [[ -n "$engine_url" && ... ]] || die 'cannot read the pinned Docker Engine identity'. If engine.json isn't found at either candidate path, this function silently prints an empty string.

The one call site that matters is line 522: push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/$(engine_archive_name)". With an empty name that becomes push LOCAL /data/local/tmp/, and adb push to a directory path pushes the file under its local basename (docker-engine.tgz), not the pinned archive name kernelctl/prepare-engine expect on the device. That would silently mis-name the sideloaded archive and only surface as a confusing failure much later (engine "not found", falls back to a network download or fails outright) instead of failing fast here.

In practice build-simple-package.sh always copies tools/engine.json alongside install.sh now, so this looks unreachable via the normal packaging path today — but it's a latent trap if that invariant ever changes (e.g. someone re-copies only install.sh/payload/ without engine.json). Worth adding the same die guard here for defense in depth.

Comment on lines +183 to +190
engine_archive_name() {
local engine_json=$SCRIPT_DIR/engine.json url name
[[ -f "$engine_json" ]] || engine_json=$SCRIPT_DIR/../tools/engine.json
url=$(engine_tarball_block "$engine_json" 2>/dev/null | sed -n 's/.*"url": "\([^"]*\)".*/\1/p')
name=${url##*/}
[[ -n "$name" ]] || die 'cannot read the pinned Docker Engine identity'
printf '%s' "$name"
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new die guard here can't actually stop the installer — it only kills a subshell.

engine_archive_name() is only ever invoked via command substitution:

push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/$(engine_archive_name)"   # line 524
phone "rm -f /data/local/tmp/$(engine_archive_name) /data/local/tmp/Image-CP2A.260805.005.lz4 ..."  # line 540

$(...) always forks a subshell to capture stdout. When name is empty (missing/malformed engine.json, or the fallback $SCRIPT_DIR/../tools/engine.json also absent), die 'cannot read the pinned Docker Engine identity' calls exit 1 — but that only terminates the subshell. The parent script never sees the failure: the exit status of a command substitution embedded mid-word is discarded (it's not a bare assignment, so set -e doesn't fire on it either).

The visible effect: the diagnostic prints to stderr, but the script keeps going with an empty substitution, so:

  • line 524 becomes push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/" — adb pushes into that directory under the source basename (docker-engine.tgz), not the pinned name kernelctl/prepare-engine on the module side expect.
  • line 540 becomes rm -f /data/local/tmp/ ... — a silent no-op on the directory (-f swallows the "is a directory" error).

So the exact safety check this PR adds ("guard the pin name") is dead code in the one case it exists to catch, and a broken bundle fails later with a confusing device-side error instead of the clean die message here.

Contrast with hash_file/verify_file elsewhere in this same script, which are always called as plain statements (never inside $(...)) specifically so their sets/exits reach the caller — engine_archive_name breaks that existing convention.

Suggested fix: capture into a variable and check explicitly at each call site, e.g. name=$(engine_archive_name) || die '...'; push ... "/data/local/tmp/$name", rather than interpolating the call directly into a larger string.

Comment thread deployment/simple-install.sh Outdated
stage 'Installing the Pixel Docker host' 'Check the module output above, package inputs, USB connection, and available phone storage.'
if [[ -f "$PAYLOAD/docker-engine.tgz" ]]; then
push "$PAYLOAD/docker-engine.tgz" /data/local/tmp/docker-29.8.0.tgz
push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/$(engine_archive_name)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

engine_archive_name's die (line 188: [[ -n "$name" ]] || die 'cannot read the pinned Docker Engine identity') is ineffective at both of its call sites (here and line 540), because it's invoked via $(engine_archive_name) nested inside a larger string argument rather than as a standalone command/assignment.

exit inside a $(...) command substitution only terminates that subshell — it does not trip set -e for the enclosing push/phone statement when the substitution is just one part of a word being built. So if tools/engine.json is ever missing from both $SCRIPT_DIR/engine.json and $SCRIPT_DIR/../tools/engine.json (e.g. an install.sh copied out of the bundle without its sibling engine.json), die prints nothing useful to the terminating flow and $(engine_archive_name) just evaluates to an empty string:

  • Line 524 becomes push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/" — since the destination is a bare directory, adb push lands the file at /data/local/tmp/docker-engine.tgz (the local file's basename), which does not match the docker-<version>.tgz name that the module's own installer-inputs.tsv/prepare-engine sideload check expects. The real failure then surfaces confusingly deep inside ksud module install/install-host instead of as this script's clear pre-flight error.
  • Line 540 becomes phone "rm -f /data/local/tmp/ /data/local/tmp/Image-CP2A.260805.005.lz4 ...", i.e. rm -f targeting a bare directory. That can make the whole phone "rm -f ..." statement return non-zero, which (unlike the swallowed inner die) does trip the script's set -e — aborting right after kernelctl install but before the adb reboot, potentially leaving the device with the new module written but not yet activated.

Suggested fix: resolve the name via a plain assignment first, so the failure actually propagates:

engine_name=$(engine_archive_name)
push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/$engine_name"
...
phone "rm -f /data/local/tmp/$engine_name /data/local/tmp/Image-CP2A.260805.005.lz4 /data/local/tmp/eip-pixel8a-forge.zip"

(A plain var=$(cmd) assignment's exit status is the substitution's exit status under set -e, unlike the well-known local var=$(cmd) masking gotcha.)

This only bites if the engine.json sibling file is missing, which build-simple-package.sh's new cp .../tools/engine.json "$OUTPUT/engine.json" line prevents for normal packaging — but it silently defeats the one guard meant to catch a broken/partial distribution, which is exactly the failure mode a pinned-identity check like this exists to catch.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Reviewed this PR's diff (README, deployment/build-simple-package.sh, deployment/simple-install.sh, and the installer test suite) against DEVICE.json and tools/engine.json, focused on KernelSU module lifecycle, Docker Engine packaging, and boot safety.

One high-confidence bug, left as an inline comment on deployment/simple-install.sh: engine_archive_name()'s new die guard for a missing/malformed pinned identity can never actually abort the installer, because the function is only ever called inside $(...) command substitution — exit there only kills the subshell, so the script silently continues with an empty archive name instead of failing cleanly.

Everything else checked out:

  • Device/build gating (akita, CP2A.260805.005, matching fingerprint) is unchanged and still consistent with DEVICE.json.
  • The module byte-verification control flow (verify_module_files) runs exactly once per install/update path, always while Docker is stopped, and correctly avoids double-verification or verifying before the reboot that merges modules_update into modules.
  • The stale-file pruning step protects KernelSU's module-state marker files (disable/remove/update/skip_mount) from deletion.
  • tools/engine.json is now bundled by build-simple-package.sh unconditionally, and it's public metadata (URL/size/sha256) — no credentials or firmware are newly introduced.
  • The bundled docker-engine.tgz push path skips client-side hash verification, but module/bin/prepare-engine independently re-verifies the sideloaded archive's identity on-device before it's ever used, so this isn't a silent-corruption risk — just an earlier client-side check that's skipped when the archive is already bundled.

No other actionable correctness or regression issues found; nothing here touches ext4 loop mounting, Wi-Fi routing, or kernel config, so those areas are unaffected by this change.

Comment on lines +183 to +188
engine_archive_name() {
local engine_json=$SCRIPT_DIR/engine.json url
[[ -f "$engine_json" ]] || engine_json=$SCRIPT_DIR/../tools/engine.json
url=$(engine_tarball_block "$engine_json" 2>/dev/null | sed -n 's/.*"url": "\([^"]*\)".*/\1/p')
printf '%s' "${url##*/}"
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

engine_archive_name() can never fail, so its callers' || die 'cannot read the pinned Docker Engine identity' guards (lines 522 and 539) are dead code.

If engine.json is unreadable — both $SCRIPT_DIR/engine.json and $SCRIPT_DIR/../tools/engine.json missing/malformed — url parses to empty, and the function's last statement, printf '%s' "${url##*/}", prints an empty string but still exits 0. The $(...) substitution at each call site therefore "succeeds" with an empty value:

  • Lines 522-523: push "$PAYLOAD/docker-engine.tgz" "/data/local/tmp/$bundled_engine_name" silently becomes push ... /data/local/tmp/. adb push to an existing directory preserves the source basename, so the file lands as /data/local/tmp/docker-engine.tgz instead of the pinned identity's filename (docker-<version>.tgz) that module/bin/prepare-engine's sideload lookup ($SIDELOAD_ROOT/$TARBALL_NAME) expects.
  • Lines 539-540: phone "rm -f /data/local/tmp/ /data/local/tmp/Image-....lz4 ..." attempts rm -f on a bare directory path. Under set -euo pipefail, a non-zero exit from that phone call aborts the installer right after the post-install reboot, with a failure message that never surfaces the real cause (unreadable engine.json).

Compare with ensure_engine_archive() a few lines below, which correctly guards the identical parse with [[ -n "$engine_url" && -n "$engine_size" && -n "$engine_sha" ]] || die .... engine_archive_name() needs the same validation (or should exit non-zero when url is empty) so the || die at each call site actually fires.

Reachability today is narrow — build-simple-package.sh now always copies tools/engine.json into the package next to install.sh, so a normally-built release won't hit this — but it's a real gap in the intended defensive check, and the failure mode if it ever does trigger (silent wrong filename, or an opaque set -e abort) is worse than the clear die the code was clearly meant to produce.

Comment thread deployment/simple-install.sh Outdated
push "$work/files.tar" /data/local/tmp/eip-module-files.tar
push "$work/manifest" /data/local/tmp/eip-module-manifest
push "$work/names" /data/local/tmp/eip-module-names
phone 'tar -xf /data/local/tmp/eip-module-files.tar -C /data/adb/modules/eip-pixel8a-forge && chown -R 0:0 /data/adb/modules/eip-pixel8a-forge && cd /data/adb/modules/eip-pixel8a-forge && /data/adb/ksu/bin/busybox sha256sum -c /data/local/tmp/eip-module-manifest -s && find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do case "$stale" in disable|remove|update|skip_mount) continue ;; esac; rm -f "$stale"; done; rc=$?; rm -f /data/local/tmp/eip-module-files.tar /data/local/tmp/eip-module-manifest /data/local/tmp/eip-module-names; exit $rc' \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale-module-file pruning can silently no-op instead of failing.

This remote command runs under plain su -c '...' with no pipefail (that only applies to the local install.sh process, not this string). Its final segment is a pipeline whose last stage is a while IFS= read -r stale; do ...; done loop:

find . -type f | sed "s|^\\./||" | LC_ALL=C sort | LC_ALL=C comm -23 - /data/local/tmp/eip-module-names | while IFS= read -r stale; do ...; done

Per POSIX, a while loop's exit status is that of the last executed body statement, or 0 if the body never ran — which happens whenever the loop reads zero lines, including when an earlier pipeline stage (find/sed/sort/comm) fails or produces no output for any reason. Since rc=$? is taken right after this pipeline, a failure anywhere in find | sed | sort | comm (not just "nothing to prune") is indistinguishable from success: the command still exits 0 and verify_module_files reports success without pruning anything.

This is made more likely by comm being the one utility in this command that, unlike sha256sum here, is not routed through /data/adb/ksu/bin/busybox — every other non-trivial utility call added in this PR uses the bundled busybox explicitly, and comm/sort are not exercised anywhere else in this repo's on-device scripts (only sed/find, which are already proven to work via toybox elsewhere). If comm isn't available as a toybox applet on this device, this failure mode would trigger on every single update.

Worth noting: tests/simple-installer.test.mjs's fake tool intercepts any command starting with tar -xf /data/local/tmp/eip-module-files.tar and substitutes its own host-side simulation (tar extract + sha256sum -c only) — it never actually executes this find | sed | sort | comm | while tail, so the test suite can't currently catch a bug here.

Suggest: route comm (and ideally sort) through /data/adb/ksu/bin/busybox for consistency with the sha256sum call in the same command, and capture/propagate the pruning pipeline's own status explicitly (e.g. capture find's/comm's exit code before the while, or restructure so a stage failure can't be masked by the terminal loop) rather than relying on the pipeline tail's exit status.

@claude

claude Bot commented Sep 15, 2026

Copy link
Copy Markdown

Reviewed against the akita/CP2A.260805.005 target and DEVICE.json. Focused on boot safety, KernelSU module lifecycle, Docker Engine pinning, and accidental artifact inclusion.

Checked and consistent:

  • tools/engine.json's pinned Docker version (29.8.0) matches DEVICE.json's docker.version; engine_archive_name()/ensure_engine_archive() fail closed (die) if the pin is missing/malformed rather than silently proceeding.
  • engine_tarball_block()'s sed range correctly isolates the tarball object so the binaries block's own size/sha256 keys can't be parsed by mistake — verified against the actual tools/engine.json contents.
  • curl download enforces --proto '=https' --proto-redir '=https' plus size and SHA-256 verification before pushing to the phone.
  • verify_module_files()'s manifest/prune logic uses LC_ALL=C sort on both the host-built names file and the phone-side find output, avoiding a classic locale-mismatch comm bug. The /data/adb/ksu/bin/busybox path matches the existing convention already used in module/*.sh.
  • Confirmed via eip/eip-hostctl.sh (park_system → stop_exact_daemon) that parking genuinely stops the Docker daemon before verify_module_files() overlays the KernelSU module tree in the existing-install/module-current path, so the "Docker-stopped window" claim in the README addition holds.
  • All three verify_module_files() call sites (post-reboot fresh/changed-module install, resumed fresh install, post-park existing install) run before Docker/Forge restart, so a verification failure aborts cleanly without leaving stale module code silently running — and doesn't risk Android boot itself, since the kernel/KSU install already completed and rebooted successfully by that point.
  • No firmware, credentials, or generated artifacts are introduced — the diff is limited to README.md, deployment/build-simple-package.sh (adds the pre-existing tools/engine.json to the package output), deployment/simple-install.sh, and the test file.
  • Confirmed the old hardcoded docker-29.8.0.tgz literal is fully gone from simple-install.sh, matching the new test assertion.

Minor, non-blocking observation: the if [[ -f "$PAYLOAD/host-module.zip" ]] guards before the two non-reboot verify_module_files calls (around the "Preparing Docker storage" and post-park stages) are vacuously true — host-module.zip is already required unconditionally by the payload file-presence check earlier in the script. Not a functional bug, just dead conditionality.

No boot-safety, device/build-assumption, kernel-config, ext4-mount, Docker-startup, or Wi-Fi-routing regressions found. No actionable blocking issue.

@exploitintel
exploitintel merged commit 0507921 into main Sep 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant