unix/weak: pass arguments to syscall at the given type

[RalfJung]

Given that we know the type the argument should have, it seems a bit strange not to use that information.

r? @m-ou-se @cuviper

[m-ou-se]

It looks like Rustc is already very restrictive about the types you can pass to a C-variadic function. It doesn't even allow a u8, which the current syscall!{} code with as c_long does allow:

extern "C" {
	fn a(_: i32, ...);
}

fn main() {
	unsafe { a(123, 0u8) };
}

error[E0617]: can't pass `u8` to variadic function
 --> src/main.rs:7:10
  |
7 |         a(123, 0u8);
  |                ^^^ help: cast the value to `c_uint`: `0u8 as c_uint`

(C happily passes a char to a variadic function, possibly because it get implicitly converted to int.)

We should try to figure out if this restriction is too strict or just right. (I don't think there's any syscall taking a char as argument. They're probably all int, long or a pointer.)

If this built-in restriction is not too loose or strict, we can just remove the as c_long instead of adding as $t.

[m-ou-se]

Seems like the original as c_long was introduced here: #56779 (comment)

[RalfJung]

That check is here:

rust/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

Lines 372 to 375 in 2bf93bd

	// There are a few types which get autopromoted when passed via varargs
	// in C but we just error out instead and require explicit casts.
	let arg_ty = self.structurally_resolved_type(arg.span, arg_ty);
	match arg_ty.kind() {

Looks like the Miri-side check that all arguments have Scalar ABI is actually redundant.^^
EDIT: No it is not, this just blocks a few types but allows by default.

[RalfJung]

Seems like the original as c_long was introduced here: #56779 (comment)

@alexcrichton @adrian-budau why was this cast introduced in the first place, do you remember?

[alexcrichton]

That was 2 years ago so I'm not entirely sure of the context, but I think I just commented how an as would work instead of a trait. As for whether or not to use a c_long, I'm not sure why myself.

[adrian-budau]

As best as I can remember: It wouldn't compile if I didn't cast some arguments. The standard says all arguments should fit in the register size, so casting to c_long seemed a sane solution. As @m-ou-se said, the compiler is pretty restrictive in terms of what you can pass to C variadic function.

[RalfJung]

Well, it seems to compile fine though...
Or is the idea that even using types like u8 (that need casting) should be supported by the syscall! wrapper? Are there truly syscalls that have such a type?

[adrian-budau]

It wasn't my intention to support such types (u8). If casting to the proper types compiles I support this.

[m-ou-se]

There's only five uses of syscall!{} in the standard library: statx, fclonefileat, copy_file_range, splice, and getrandom. For those it works fine without any cast. Looking at https://elixir.bootlin.com/linux/latest/source/include/linux/syscalls.h, it seems like it'd work fine for every Linux syscall.

the compiler is pretty restrictive in terms of what you can pass to C variadic function.

this just blocks a few types but allows by default.

Looks like it isn't very restrictive. Even passing a String or Vec or NonZeroU8 compiles fine. Only 8 and 16 bit integers, bool, f32, and functions are blocked.

[cuviper]

RE: #78785 (comment)

Isn't it UB when the caller and callee types do not match?

The callee libc::syscall doesn't really have types at all! It doesn't try to interpret anything, as the whole point is that it allows syscalls that aren't known to your libc. It's really just a shim that translates arguments to the kernel syscall ABI registers, and then captures errno on return.

It's hard to even puzzle this out from glibc sources, because all arches are implemented directly in assembly, but in musl you can see that it simply reads 6 unconditional syscall_arg_t (long) arguments from the va_list. I think that's where the idea came to cast them in #56779, since that mentions musl.

https://git.musl-libc.org/cgit/musl/tree/src/misc/syscall.c

I think it's not strictly necessary to cast the arguments like that, as long as you follow the C promotion rules for varargs then it will work out, but it seems on the safer side to just pass long to begin with.

If casting to the proper types compiles I support this.

If we choose not to cast to c_long, then I would avoid casting anything at all. We're already wrapping this in our own function with the correct argument types.

unsafe fn $name($($arg_name:$t),*) -> $ret {

[RalfJung]

The callee libc::syscall doesn't really have types at all! It doesn't try to interpret anything, as the whole point is that it allows syscalls that aren't known to your libc. It's really just a shim that translates arguments to the kernel syscall ABI registers, and then captures errno on return.

Seems like I am too attached to type systems to fully wrap my head around this.^^

If you prefer we can also hack something in Miri to accept both 32bit and 64bit integers here (I presume we should truncate them to 32bit as who knows which junk might be in the "upper half" in case the caller just passed 32bit).

[cuviper]

Can you explain the issue from Miri's side? Why does it care?

[cuviper]

who knows which junk might be in the "upper half" in case the caller just passed 32bit

That seems like an argument for casting to c_long, because then you get well-defined upper bits, 0 for unsigned or else sign-extended. I'm not sure if the kernel side really cares, but I vaguely recall some architectures do need proper sign extended values -- powerpc maybe? That might happen naturally through varargs in the normal case, but I don't know.

[RalfJung]

Can you explain the issue from Miri's side? Why does it care?

When Miri convert an interpreter value to something it works on, it needs to state the size that the value should have. We always double-check that the actual size of the value matches what the interpreter expects. In many cases this is required as mismatching size is UB; furthermore this approach has caught countless bugs.

That seems like an argument for casting to c_long, because then you get well-defined upper bits, 0 for unsigned or else sign-extended. I'm not sure if the kernel side really cares, but I vaguely recall some architectures do need proper sign extended values -- powerpc maybe? That might happen naturally through varargs in the normal case, but I don't know.

So Miri should require the type to be c_long? Then some other code needs to be adjusted I think.

Miri needs to be written in a way such that if it does not complain about UB, whatever it does is guaranteed to match whatever happens on any possible real implementation. This is easy to do when we enforce that the size exactly matches what is says in the syscall manpage. If we truly want to support code that relies on further details of the ABI, we need to be really careful to not accidentally accept incorrect code.

Right now, the approach in Miri when implementing some FFI function or syscall is that we check the manpage and enforce that all argument types match what it says there. This is easy to explain and obviously sound (in the sense that we will not accept incorrect code). If we diverge from that I'd like to make sure I understand how and why it is okay to do so.

[cuviper]

Right now, the approach in Miri when implementing some FFI function or syscall is that we check the manpage and enforce that all argument types match what it says there.

But I don't understand what you're trying to match against in this case, when we're calling syscall:

long syscall(long number, ...);

Is Miri trying to interpret that number and type-check the varargs? If so, I think that's too aggressive.

[cuviper]

I guess this is regarding the syscall interception here?
https://github.com/rust-lang/miri/blob/df4109151b6870cdb6d170326d1c099746990ea8/src/shims/posix/linux/foreign_items.rs#L114-L115

I'm not sure what to suggest about that...

[m-ou-se]

An option could be to avoid syscall(long, ..) as much as possible. std now tries to use dlsym("getrandom") (and "statx", etc.) before using the variadic syscall wrapper (except for SYS_futex, which doesn't have a libc function). Maybe Miri could make the dlsym function succeed instead of returning null. The function obtained through that will be called by std with a full signature, instead of a variadic one.

[RalfJung]

Is Miri trying to interpret that number and type-check the varargs? If so, I think that's too aggressive.

How is that too aggressive? At a conceptual level this is what the implementation also does, if we do not want to rely on ABI details. It matches on the syscall number and then interprets the arguments.

Are you saying the syscall manpages's use of types is entirely meaningless? man 2 getrandom says

ssize_t getrandom(void *buf, size_t buflen, unsigned int flags);

I guess this is regarding the syscall interception here?

This specific problem arises here:

https://github.com/rust-lang/miri/blob/df4109151b6870cdb6d170326d1c099746990ea8/src/shims/posix/linux/foreign_items.rs#L211

(The flag value actually does not matter, so the only reason we call to_i32 is UB detection, but in many other cases we need to know the value of the argument to be able to correctly emulate this function call.)

[cuviper]

Could Miri just validate that whatever argument type is there, that it's legal to cast as the type the emulated syscall wants? And then convert it as needed.

man 2 getrandom says

We're not calling man 2 getrandom, we're calling man 2 syscall. The fact that the userspace getrandom matches the types of the raw SYS_getrandom is nice, but that's not a guarantee in general. For example, clone is a bit of a mess in how it maps to different arch-specific syscalls -- see clone NOTES.

[cuviper]

I guess my point is that syscall type validation is a kernel concern, and I doubt UB detection really makes sense across that boundary. I guess you could get corrupted memory if you pass a bad pointer that the kernel will write, but I think that's different than what you're checking, right?

[RalfJung]

I guess my point is that syscall type validation is a kernel concern, and I doubt UB detection really makes sense across that boundary.

That's fair. However, I assume that if the type is, e.g., long, but the program passes int, then this is UB on a 64bit arch as the "higher bits" might be anything. So I feel like we need to at least check a lower bound for the size of the type.

And 128bit types might have a different calling convention entirely. Can we accept those as well?

And furthermore, I think this means we rely on the fact that for all ABIs, when you pass a 64bit value to a syscall which expects a 32bit argument, things will just get truncated. I cannot tell if that is obviously true for every single ABI that Rust supports now or in the future.

I hope you see why I don't consider it easy to just accept two different sizes of arguments for one function/operation.^^

[cuviper]

That's fair. However, I assume that if the type is, e.g., long, but the program passes int, then this is UB on a 64bit arch as the "higher bits" might be anything. So I feel like we need to at least check a lower bound for the size of the type.

When we currently cast int to c_long, the higher bits are well defined.

From Miri's side, that sounds reasonable to check the lower bound size, so you know the "useful" bits are well defined -- especially if you're considering syscall callers outside of our control.

And 128bit types might have a different calling convention entirely. Can we accept those as well?

There are concerns in man 2 syscall about values that are greater than register size. I don't think anything uses 128-bit, and if there were it would probably be passed as an indirect pointer, but there are examples of 64-bit args that are a problem for 32-bit targets. Simple casting won't solve those problems, but we're not using any of those cases now.

And furthermore, I think this means we rely on the fact that for all ABIs, when you pass a 64bit value to a syscall which expects a 32bit argument, things will just get truncated. I cannot tell if that is obviously true for every single ABI that Rust supports now or in the future.

We already have a typed $arg_name:$t to start with. I think we can either leave that as-is, or keep casting to c_long like explicit syscall register args, but the round-trip $arg_name as c_long as $t makes no sense to me.

[RalfJung]

We're not calling man 2 getrandom, we're calling man 2 syscall. The fact that the userspace getrandom matches the types of the raw SYS_getrandom is nice, but that's not a guarantee in general.

Oh, I thought this would be docs about the underlying syscall. Thanks for pointing that out. Where do I find information for the types expected by the individual syscalls?

When we currently cast int to c_long, the higher bits are well defined.

My question wasn't about what to do in this specific case, but what to do in general. Miri has to work not just for whatever the standard library currently happens to do on x86_64.

Or I guess we could make Miri accept 64bit arguments but requires the higher bits to be 0, but that seems... odd?

There are concerns in man 2 syscall about values that are greater than register size. I don't think anything uses 128-bit, and if there were it would probably be passed as an indirect pointer, but there are examples of 64-bit args that are a problem for 32-bit targets. Simple casting won't solve those problems, but we're not using any of those cases now.

"there are examples of 64-bit args that are a problem for 32-bit targets" sounds like we should not accept 64bit arguments for 32bit parameters in general. Maybe ptr-sized arguments for 32bit parameters work in general, at least if the ptr size is >= 32bit?

We already have a typed $arg_name:$t to start with. I think we can either leave that as-is, or keep casting to c_long like explicit syscall register args, but the round-trip $arg_name as c_long as $t makes no sense to me.

That's fair, I can remove the roundtrip.

But that does not answer my question: can we be sure that, in any ABI Rust supports now or in the future, passing a c_ulong-sized variadic argument (I said 64bit before, sorry for this) instead of a c_uint-sized variadic argument, will effectively pass the argument truncated to c_uint? That's what this code here is relying on, after all, and that is what Miri would need to implement to make this code work (unless we're landing this PR).

[cuviper]

Oh, I thought this would be docs about the underlying syscall. Thanks for pointing that out. Where do I find information for the types expected by the individual syscalls?

I may have spoken too quickly -- man 2 is indeed the section for syscalls, but it's showing the userspace ideal of sorts, like in the clone mess. Plus there's the fact that man 2 syscall exists, but there's no such thing as SYS_syscall... it's a fuzzy line to me.

The pure reference would be in the kernel source:

• include/linux/syscalls.h
• include/asm-generic/syscalls.h
• plus various arch/*/include/asm/syscalls.h

Then if you look at the example getrandom in linux/drivers/char/random.c:

SYSCALL_DEFINE3(getrandom, char __user *, buf, size_t, count,
		unsigned int, flags)

That wrapper can vary between arches, but the generic one defines asmlinkage long __se_sys_##name with all long args, and that function casts the args to their declared types and calls the real definition in inline long __do_sys##name. So the actual user-to-kernel transition happens with long args.

My question wasn't about what to do in this specific case, but what to do in general. Miri has to work not just for whatever the standard library currently happens to do on x86_64.

But you're not trying to emulate all possible syscall(SYS_*, ...) in general, just a few useful ones.

Or I guess we could make Miri accept 64bit arguments but requires the higher bits to be 0, but that seems... odd?

I guess you could treat it sign-extended, so all ones are also fine when expecting a smaller signed type.

"there are examples of 64-bit args that are a problem for 32-bit targets" sounds like we should not accept 64bit arguments for 32bit parameters in general. Maybe ptr-sized arguments for 32bit parameters work in general, at least if the ptr size is >= 32bit?

I'd say you should not accept anything larger than the target size -- not because it's impossible, but because that presents a challenge we don't need to deal with right now.

64-bit arguments for 32-bit parameters are fine on 64-bit targets, just using a full register.

But that does not answer my question: can we be sure that, in any ABI Rust supports now or in the future, passing a c_ulong-sized variadic argument (I said 64bit before, sorry for this) instead of a c_uint-sized variadic argument, will effectively pass the argument truncated to c_uint? That's what this code here is relying on, after all, and that is what Miri would need to implement to make this code work (unless we're landing this PR).

I'm saying they are effectively passed to the kernel in registers (~long) no matter what we do, and libc::syscall is only translating arguments from the variadic C ABI to that kernel syscall ABI without knowing the "real" types. I can't predict the future, but given that syscall must work with unknown SYS_foo, I don't see how this could be any different.

[cuviper]

All that said, I'm okay with dropping the cast, assuming it gets through CI of course. If it breaks, then I guess we'll find out why #56779 wanted it, and we can instead add a comment explaining this to our future selves.

[RalfJung]

That wrapper can vary between arches, but the generic one defines asmlinkage long _se_sys##name with all long args, and that function casts the args to their declared types and calls the real definition in inline long __do_sys##name. So the actual user-to-kernel transition happens with long args.

I'm saying they are effectively passed to the kernel in registers (~long) no matter what we do, and libc::syscall is only translating arguments from the variadic C ABI to that kernel syscall ABI without knowing the "real" types. I can't predict the future, but given that syscall must work with unknown SYS_foo, I don't see how this could be any different.

I see, thanks for digging that up. This almost sounds like Miri should require all arguments to be ptr-sized.^^

So it is the other way around then -- if we pass things as c_int we are relying on int-sized arguments to be passed the same as ptr-sized arguments (with potentially some junk in the upper bytes that won't matter as the syscall will truncate inside the kernel).

All that said, I'm okay with dropping the cast, assuming it gets through CI of course. If it breaks, then I guess we'll find out why #56779 wanted it, and we can instead add a comment explaining this to our future selves.

So, that'd be landing the PR as-is (I removed the roundtrip), and only taking further action if that fails in bors?
@bors rollup=iffy

[m-ou-se]

@bors r+

I don't expect any problems with this change. There's very few usages of this macro, and it only affects Linux.

(It will break getrandom and statx in Miri though.)

[bors]

📌 Commit d8d763d has been approved by m-ou-se

[bors]

⌛ Testing commit d8d763d with merge acaf1ab89e3cfe64f42b879fd87813bbc979f6df...

[RalfJung]

Not getrandom, that one is currently agnostic about the size, but possibly statx.

[bors]

⌛ Testing commit d8d763d with merge 3d61742233888555f7a33bb32b0277b1070d5af4...

[bors]

⌛ Testing commit d8d763d with merge 172acf8...

[m-ou-se]

Bors, what are you doing? (:

I edited the r+ comment and now it's testing the PR twice?

[RalfJung]

Editing the comment re-sends the command. Probably it interpreted the re-r+ as a retry.

[RalfJung]

(That is the first job being cancelled, I guess?)

[bors]

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 172acf8 to master...

[rust-highfive]

📣 Toolstate changed by #79196!

Tested on commit 172acf8.
Direct link to PR: #79196

💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).