Syscall rewriter: interface changes, no_std support, and hardening#738
Syscall rewriter: interface changes, no_std support, and hardening#738
Conversation
b9cfe00 to
913c8b3
Compare
dacdaf5 to
d377b78
Compare
Add skipped_addrs output parameter to hook_syscalls_in_elf for reporting unpatchable syscall locations. Add no_std/alloc support with BTreeSet replacing HashSet for deterministic iteration. Add Bun executable detection (UnsupportedBunExecutable error), ET_REL object file rejection, phdr alignment fixup, fork-to-vfork patching, UD2 replacement for unpatchable syscalls, found_any tracking to move NoSyscallInstructionsFound to inner function, and println! removal. Add patch_code_segment public API for in-place code patching. Add unit tests for new functionality and update snapshot tests with address normalization. Update all callers (runner, optee, packager) to 3-arg API.
d377b78 to
3719609
Compare
Reject unsupported Bun executables during packaging, keep rewritten trailer semantics compatible with the loader, and turn malformed rewrite failures into explicit errors.
Match CI rustfmt output for the x86 trampoline comments in the rewritten ELF path.
litebox_syscall_rewriter/src/lib.rs
Outdated
| if off + 5 <= buf.len() && patch_end <= buf.len() && off + 5 <= patch_end { | ||
| buf[off] = 0xE9; // JMP rel32 | ||
| buf[off + 1..off + 5].copy_from_slice(&rel32.to_le_bytes()); | ||
| } else { |
There was a problem hiding this comment.
Two potential issues:
- Overwriting
endbr64(iffork_file_offsetis the starting address offork) - No NOP-filling
litebox_syscall_rewriter/src/lib.rs
Outdated
| ) -> Option<(u64, u64, i32)> { | ||
| use object::ObjectSymbol as _; | ||
|
|
||
| let mut fork_vaddr = None; |
There was a problem hiding this comment.
We might need to patch _Fork as well.
There was a problem hiding this comment.
I'm not sure about this one.
There was a problem hiding this comment.
Both libc_fork and _Fork eventually invoke the fork syscall. The difference is whether they call pthread_atfork or not (mostly libc or other libraries' internal behaviors). Speaking of pthread_atfork, doing not call this function might result in library internal consistency break. https://docs.openssl.org/3.2/man3/OPENSSL_fork_prepare/ libc_vfork doesn't call atfork, so our rewriter might need to handle this.
There was a problem hiding this comment.
I will remove the fork->vfork patching. I think we should just work with the fork syscall (when we get to the support for multi processes and vfork/fork).
| // input, not executable code. Rewriting instructions or appending | ||
| // trampoline data would corrupt the object file for the linker. | ||
| // Check the ELF e_type field (bytes 16..18) before doing any work. | ||
| if input_binary.len() >= 18 { |
There was a problem hiding this comment.
why do we run this rewriter against .o?
There was a problem hiding this comment.
This is defense in depth. The broker right now just checks if it's an elf file so it may call the rewriter with a .o file.
jaybosamiya-ms
left a comment
There was a problem hiding this comment.
I have not done a deep dive into reviewing this PR, but I noticed a few things when looking through it. I do think that the rewriter would benefit from a cleanup pass in the future, so for now, I do not want to block PRs to tweak it to get no_std support.
| fn checked_add_u64(base: u64, addend: u64, context: &'static str) -> Result<u64> { | ||
| base.checked_add(addend) | ||
| .ok_or_else(|| Error::ParseError(format!("{context} address overflow"))) | ||
| } | ||
|
|
||
| fn checked_sub_u64(base: u64, subtrahend: u64, context: &'static str) -> Result<u64> { | ||
| base.checked_sub(subtrahend) | ||
| .ok_or_else(|| Error::ParseError(format!("{context} address underflow"))) | ||
| } | ||
|
|
||
| fn rel32_bytes(target: u64, base: u64, context: &'static str) -> Result<[u8; 4]> { | ||
| let disp = i128::from(target) - i128::from(base); | ||
| let disp = i32::try_from(disp).map_err(|_| { | ||
| Error::ParseError(format!( | ||
| "{context} displacement out of range: target {target:#x}, base {base:#x}" | ||
| )) | ||
| })?; | ||
| Ok(disp.to_le_bytes()) | ||
| } |
There was a problem hiding this comment.
Nit: I'm not fully sure why these are ParseError. These seem to be helpers that indeed do the right thing for the current callers (afaict), but the names imply more general, at which point the errors would be actively annoying.
There was a problem hiding this comment.
I don't fully understand your comment here. Did you mean to replace ParseError with something like PatchError?
There was a problem hiding this comment.
It is a bit of a nit, it is that checked_sub_u64 (for example) does not indicate anything about it being parsing related, but it doing a ParseError is surprising, that's all.
It is fine to leave it as-is.
…riants, use ICEBP;HLT trap - Change hook_syscalls_in_elf and patch_code_segment to return (Vec<u8>, Vec<u64>) instead of taking &mut Vec<u64> for skipped addresses - Rename UnsupportedBunExecutable to UnsupportedExecutable(String) and add context to UnsupportedObjectFile(String) - Add PatchError variant for address arithmetic failures (previously ParseError) - Replace UD2 (0F 0B) with ICEBP;HLT (F1 F4) for poisoned syscalls to avoid confusion with unreachable!() paths - Make anyhow/clap features use dep: syntax in Cargo.toml - Update all callers in packager, runner, and tests
|
🤖 SemverChecks 🤖 Click for details |
| /// not trap on Linux in userspace, but `HLT` does (SIGILL in ring 3), and the | ||
| /// `F1` prefix makes it easy for a signal handler to distinguish an | ||
| /// intentional break from a spurious one. |
There was a problem hiding this comment.
This comment is clearly incorrect.
$ cat x.c
int main(void) {
__asm__ volatile("hlt");
return 0;
}
$ gcc -o x x.c -g
$ gdb ./x
Reading symbols from ./x...
(gdb) r
Starting program: /tmp/tmp.2026-04-07/15-35-06/polished-lumpsucker/x
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Program received signal SIGSEGV, Segmentation fault.
main () at x.c:2
2 __asm__ volatile("hlt");| if p_type == object::elf::PT_PHDR { | ||
| // PT_PHDR — update p_offset to match new location | ||
| let p_offset_off = entry_off + 8; | ||
| let old_p_offset = | ||
| u64::from_le_bytes(buf[p_offset_off..p_offset_off + 8].try_into().unwrap()); | ||
| if old_p_offset == e_phoff { | ||
| let new_p_offset = (old_p_offset + padding as u64).to_le_bytes(); | ||
| buf[p_offset_off..p_offset_off + 8].copy_from_slice(&new_p_offset); | ||
| } | ||
| // The PHDR segment size should match the phdr table; no change needed. | ||
| } |
There was a problem hiding this comment.
currently, this code does not update PT_PHDR.p_vaddr/PT_PHDR.p_paddr.
Add
no_stdsupport tolitebox_syscall_rewriter(behind a feature flag, usingalloc/BTreeSet). Harden ELF rewriting with input validation (Bun executables, relocatable objects), phdr alignment fixup, and checked arithmetic replacing panicking casts. Changehook_syscalls_in_elfto return skipped unpatchable syscall addresses alongside the rewritten binary, add a newpatch_code_segmentAPI for runtime in-place patching, and enrich error variants with context strings. Unpatchable syscalls are now replaced withICEBP; HLTinstead of being silently ignored.