Skip to content

check that all syscall arguments are scalars #1570

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 3, 2020

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 3, 2020

@m-ou-se do you think this check makes sense?

The abi of a layout contains everything needed for the calling convention (as far as I know), so this should ensure that all the ignored arguments are passed like integers and do not otherwise mess up argument passing.

The one thing I am not sure about is what happens when more arguments are passed than fit in registers. The syscall man page says that all calling conventions support at least 6 arguments, except for "mips/o32" where it says that

       [1] The mips/o32 system call convention passes arguments 5 through 8 on the user stack.

If we assume that passing these extra arguments to futex is legal even with that calling convention, that would mean that those user stack arguments are just silently ignored as well, which seems plausible to me -- but I know very little about calling conventions.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 3, 2020

Looks good to me!

The one thing I am not sure about is what happens when more arguments are passed than fit in registers.

At least for x86_64, those go on the stack. This already happens for the sixth argument to a syscall: when calling the libc syscall() variadic function with 7 arguments (the syscall number plus 6 arguments to the sycall), the arguments are passed to this function through:

  1. %rdi (syscall number)
  2. %rsi (arg1)
  3. %rdx (arg2)
  4. %rcx (arg3)
  5. %r8 (arg4)
  6. %r9 (arg5)
  7. 8(%rsp) (arg6) (on the stack) ← ← ←

This wrapper then needs to invoke the syscall instruction, which uses a different convention to pass the arguments:

  1. %rax (syscall number)
  2. %rdi (arg1)
  3. %rsi (arg2)
  4. %rdx (arg3)
  5. %r10 (arg4)
  6. %r8 (arg5)
  7. %r9 (arg6)

Glibc's syscall() wrapper on x86_64 has to move all these arguments over to 'convert' to the Linux syscall calling convention:

ENTRY (syscall)
	movq %rdi, %rax		/* Syscall number -> rax.  */
	movq %rsi, %rdi		/* shift arg1 - arg5.  */
	movq %rdx, %rsi
	movq %rcx, %rdx
	movq %r8, %r10
	movq %r9, %r8
	movq 8(%rsp),%r9	/* arg6 is on the stack.  */
	syscall			/* Do the system call.  */
	cmpq $-4095, %rax	/* Check %rax for error.  */
	jae SYSCALL_ERROR_LABEL	/* Jump to error handler if error.  */
	ret			/* Return to caller.  */

If we assume that passing these extra arguments to futex is legal even with that calling convention, that would mean that those user stack arguments are just silently ignored as well, which seems plausible to me -- but I know very little about calling conventions.

Yes, I'd assume the same. For the same reason It's safe to call printf("hello", 1, 2, 3, 4, 5);.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 3, 2020

At least for x86_64, those go on the stack. This already happens for the sixth argument to a syscall: when calling the libc syscall() variadic function with 7 arguments (the syscall number plus 6 arguments to the sycall), the arguments are passed to this function through:

The docs say that for x86-64, 6 arguments are passed via registers:

x86-64 rdi rsi rdx r10 r8 r9

I think this excludes the syscall number, which it says is passed via rax. Not that any of these names mean anything to me.^^

EDIT: Oh, there's the variadic wrapper and then the actual syscall? Ouch... because things were not complicated enough yet.^^

@RalfJung
Copy link
Member Author

RalfJung commented Oct 3, 2020

Glibc's syscall() wrapper on x86_64 has to move all these arguments over to 'convert' to the Linux syscall calling convention:

Doesn't this just drop arg7+ though? I guess there are no syscalls with that many arguments? But some platforms specify a register for arg7...

Yes, I'd assume the same. For the same reason It's safe to call printf("hello", 1, 2, 3, 4, 5);.

So 6 arguments are always okay; I guess what we need here is that this works for any number of extra arguments, even if they spill onto the stack.

@m-ou-se
Copy link
Member

m-ou-se commented Oct 3, 2020

I think this excludes the syscall number, which it says is passed via rax. Not that any of these names mean anything to me.^^

Yeah, the syscall number (in rax) is not considered 'an argument' to the syscall. That's why the arguments all need to be shifted. (Next to replacing rcx by r10.)

Oh, there's the variadic wrapper and then the actual syscall? Ouch... because things were not complicated enough yet.^^

Yeah :( The variadic wrapper is only variadic in the C header (and Rust's libc) declaration. The assembly implementation of it is implemented as if it just takes a fixed amount of arguments.

Doesn't this just drop arg7+ though? I guess there are no syscalls with that many arguments? But some platforms specify a register for arg7...

Yup. Afaik there's no syscalls that take more than 6 arguments, at least not on x86_64.

So 6 arguments are always okay; I guess what we need here is that this works for any number of extra arguments, even if they spill onto the stack.

Yup, they just stand there untouched. Stack arguments to variadic functions are always cleaned up by the caller.

The C standard even requires this for functions like fprintf:

ISO/IEC 9899:2011 - 7.21.6.1: (...) If the format is exhausted while arguments remain, the excess arguments are evaluated (as always) but are otherwise ignored. (...)

@RalfJung
Copy link
Member Author

RalfJung commented Oct 3, 2020

The C standard even requires this for functions like fprintf:

That is reassuring. :) Thanks!

We could probably also do something similar for other variadic functions that we implement (fcntl comes to mind), but for now that does not seem necessary.

@bors r+

@bors
Copy link
Contributor

bors commented Oct 3, 2020

📌 Commit 2b2a3a0 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Oct 3, 2020

⌛ Testing commit 2b2a3a0 with merge 3fafb83...

@bors
Copy link
Contributor

bors commented Oct 3, 2020

☀️ Test successful - checks-travis, status-appveyor
Approved by: RalfJung
Pushing 3fafb83 to master...

@bors bors merged commit 3fafb83 into rust-lang:master Oct 3, 2020
@RalfJung RalfJung deleted the syscalls branch October 3, 2020 17:46
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.

3 participants