UB if noreturn syscall actually returns

[rusty-snake]

rustix/src/backend/linux_raw/arch/x86_64.rs

Lines 55 to 63 in cb01fbe

	#[inline]
	pub(in crate::backend) unsafe fn syscall1_noreturn(nr: SyscallNumber<'_>, a0: ArgReg<'_, A0>) -> ! {
	asm!(
	"syscall",
	in("rax") nr.to_asm(),
	in("rdi") a0.to_asm(),
	options(nostack, noreturn)
	)
	}

rustix/src/backend/linux_raw/runtime/syscalls.rs

Lines 143 to 146 in cb01fbe

	#[inline]
	pub(crate) fn exit_thread(code: c::c_int) -> ! {
	unsafe { syscall_noreturn!(__NR_exit, c_int(code)) }
	}

Every syscall may return

Every syscall may return with any value because of seccomp-bpf.

This includes pure syscalls like getuid that may return the current uid as 4294967295_u32 (-EPERM) or exit/exit_group returning. While such seccomp-bpf filters will have huge compatibility problems and are unlikely in real-world, they must be handled in a safe way.

In the case of exit/exit_group there are two safe possibilities that maintain -> ! to the caller:

1. Infinity loop

   // Note that this is a pseudo example. If the `sys_exit` function is `-> !`,
   // Rust is free to remove the loop because returning from `-> !` is UB.
   // The loop must be implemented in assembly with a backward jump
   // if the `asm!` is marked `noreturn` as return from the `noreturn`-`asm!`
   // is already UB.
   loop {
       sys_exit(code)
   }

2. Segfault, i.e. ud2 (x86-64) after the syscall.

[sunfishcode]

rustix also trusts that when read returns 4, that exactly 4 bytes have been written into the buffer. If someone rigs a seccomp-bpf script to make read return a greater number, rustix will execute UB when it forms a &[u8] containing uninitialized bytes.

rustix also trusts that if openat returns a non-negative number, it's a new unique fd that upholds the assumptions of I/O safety. rustix also trusts that mmap returns the pointer is what it says it is rather than a pointer to something else (yes, mmap is unsafe, but the safety comment trusts that this isn't something users need to worry about).

And these issues affect the libc backend as well as the linux_raw backend. They even affect typical Rust idioms using eg. libc::read or even std::io::Read::read and not using rustix at all.

It doesn't seem practical to defend against arbitrary theoretical seccomp_bpf scripts, in rustix, or in the Rust ecosystem as a whole.

[rusty-snake]

Yeah, I see your point.

Looking at the generated assembly rustc seems to automatically insert ud2 after a noreturn-asm!. So practically it will not fall out of syscall1_noreturn. But maybe behind the "syscall".

If someone rigs a seccomp-bpf script to make read return a greater number,
...
rustix also trusts that if openat returns a non-negative number, it's a new unique fd that upholds the assumptions of I/O safety
...
rustix also trusts that mmap returns the pointer is what it says it is rather than a pointer to something else (yes, mmap is unsafe, but the safety comment trusts that this isn't something users need to worry about).

All of them are true but there is one fundamental difference. (plain) seccomp-bpf can not spoof a successful return. It can only allow (real return from kernel), spoof an error (return value MAX_ERRNO..=-1) or trap/kill/…. Spoofing an successful return requires user-space notifications, i.e. syscall emulation in user-space by an (per-definition) trusted supervisor (debugger/tracer/emulator/micro-kernel/...). Which (for me) fall into the OS/debugger-memory-safety-exception-rule.

[sunfishcode]

I'm still thinking about this. I agree that userspace notifications are at least a plausible place to draw a line.

For functions which should never return, which is just exit and exit_group, the cost of adding trap instructions is very low, so it may make sense to just do. That said, if Rust is already adding trap instructions in practice, then the practical benefit, which was already obscure, is obscure and tiny.

Functions which rustix assumes are infallible seem like the bigger question. For rustix, the list is currently:

• __NR_uname
• __NR_umask
• __NR_sysinfo
• __NR_sync
• __NR_clock_getres (with a known constant clock id)
• __NR_getuid
• __NR_geteuid
• __NR_getgid
• __NR_getegid
• __NR_getppid
• __NR_getpgrp
• __NR_getpgid
• __NR_getpid
• __NR_gettid
• __NR_prlimit64 (with pid=0 and no new_limit)
• __NR_arch_prctl (ARCH_SET_FS)
• __NR_set_tid_address
• __NR_rt_sigpending

Many of these can be easily inlined into user code, which is less nice if the code also requires a branch to check for failure.
Most of these are explicitly documented as never failing, though admittedly a few are not (eg. prlimit) and rustix just assumes it knows that the documented failures are the only failures. Another plausible place to draw a line then is, rustix should only assume something is infallible if it is explicitly documented as such.

Overall, I don't think I have a clear view of this yet.

[rusty-snake]

That said, if Rust is already adding trap instructions in practice, then the practical benefit, which was already obscure, is obscure and tiny.

Yes, it is more a "problem" of being documented as UB at a higher level than invalid assembly.

Many of these can be easily inlined into user code, which is less nice if the code also requires a branch to check for failure.

There is enough foo(getuid()) or getuid() == 0 code out there. If the return value is just a normal integer (and the function is documented as infallible) it would be fine to just return a strange value as long as there is no niche-value/unreachable_unchecked /... involved (i.e. only correctness safety, no soundness safety). For pointers/memory access a check could be worth.

[sunfishcode]

I've now filed #1444 and #1443 to implement trapping for noreturn syscalls and to establish a policy for negative pids from getpid and similar. There's more to think about for the other functions though.