Fix slice copy range for 'want' in dtb.rs#9934
Open
schaumb wants to merge 1 commit into
Open
Conversation
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.
magiskboot dtb patchalways crashes when patchingskip_initramfs(off-by-one slice since the Rust rewrite)Summary
magiskboot dtb <file> patchpanics/aborts every time it findsskip_initramfsin thebootargsproperty of achosennode. Theskip_initramfs→want_initramfspatch cannever succeed — the process crashes on the first match, before writing anything.
Affected versions
first shipped in v26.4.
master:Magisk/native/src/boot/dtb.rs
Line 237 in 14ea5cf
Root cause
The C++ → Rust migration introduced an off-by-one in the slice range. The original C++
(
dtb.cpp, removed in the same commit) copied exactly 4 bytes:The Rust rewrite uses an inclusive range, which is 5 bytes (indices 0..=4):
<[u8]>::copy_from_slicerequires both slices to have equal length, so this lineunconditionally panics with:
Since magiskboot is built with
panic = "abort"(immediate-aborton recent master),the process aborts on the spot. On
immediate-abortbuilds it dies without evenprinting the panic message.
Steps to reproduce
Expected: prints
Patch [skip_initramfs] -> [want_initramfs] in dtb.0000, exits 0,and the dtb now contains
want_initramfs.Actual: the process panics/aborts (SIGABRT), exits non-zero, and the dtb is left
unpatched.
Impact
skip_initramfspatch has been non-functional since v26.4: it crashes in exactly(and only) the case it exists for.
scripts/boot_patch.shinvokes it asif ./magiskboot dtb $dt patch; then, so thecrash is indistinguishable from "nothing to patch" — installation continues silently
and produces a boot/dtb image where the kernel cmdline still contains
skip_initramfs, so the ramdisk (and Magisk) is skipped on legacy system-as-rootdevices that rely on this mechanism.
for_each_fdt(file, true, ...)uses a read-write memory map andpatches in place. If an earlier fdt in the blob gets its fstab verity flags patched
and a later fdt then hits the
skip_initramfsabort, the file is leftpartially patched on disk.
Fix
Use an exclusive range so the destination is 4 bytes, matching the original
memcpy: