Page MenuHomeFreeBSD

Don't let cpu_set_syscall_retval() clobber exec_setregs().
ClosedPublic

Authored by ed on Nov 21 2017, 7:15 AM.
Tags
None
Referenced Files
F103836594: D13180.id35530.diff
Sat, Nov 30, 2:04 AM
Unknown Object (File)
Sat, Nov 16, 3:53 AM
Unknown Object (File)
Tue, Nov 12, 4:34 PM
Unknown Object (File)
Oct 25 2024, 3:37 AM
Unknown Object (File)
Oct 17 2024, 5:26 PM
Unknown Object (File)
Oct 14 2024, 10:24 AM
Unknown Object (File)
Oct 6 2024, 4:09 AM
Unknown Object (File)
Oct 1 2024, 4:59 PM
Subscribers

Details

Summary

Upon successful completion, the execve() system call invokes
exec_setregs() to initialize the registers of the initial thread of the
newly executed process. What is weird is that when execve() returns, it
still goes through the normal system call return path, clobbering the
registers with the system call's return value (td->td_retval[0]).

Though this doesn't seem to be problematic for x86 most of the times (as
the value of eax/rax doesn't matter upon startup), this can be pretty
frustrating for architectures where function argument and return
registers overlap (e.g., ARM). On these systems, exec_setregs() also
needs to initialize td_retval.

Even worse are architectures where cpu_set_syscall_retval() sets
registers to values not derived from td_retval. On these architectures,
there is no way cpu_set_syscall_retval() can set registers to the way it
wants them to be upon the start of execution.

To get rid of this madness, let sys_execve() return EJUSTRETURN. This
will cause cpu_set_syscall_retval() to leave registers intact. This
makes process execution easier to understand. It also eliminates the
difference between execution of the initial process and successive ones.
The initial call to sys_execve() is not performed through a system call
context.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 12906
Build 13168: arc lint + arc unit

Event Timeline

Also look at _all_ uses of kern_execve() in the kernel. In particular, linux_common_execve().

sys/i386/i386/machdep.c
1175

This is a non-trivial comment, I think it should be preserved. Might be, in different place.

sys/kern/kern_exec.c
321

Look at this line.

ed marked an inline comment as done.

Process kib@'s feedback. Thanks!

sys/i386/i386/machdep.c
1175

Done! Moved it to the place where we zero all of the regs.

sys/kern/kern_exec.c
321

Oomf. Good catch!

Linux exec is still left with the bug.

sys/i386/i386/machdep.c
1129

Perhaps one 'sure' is enough, old code contained a typo.

sys/kern/kern_exec.c
965–971

I suggest to explain the error conversion there.

Take a look at all kern_execve() call sites. Fix up linux_common_execve().

Make sure sure this comment doesn't contain a typo.

Remove a stale comment regarding regs <-> retval.

Note my comment at the return() in do_execve().

This revision is now accepted and ready to land.Nov 21 2017, 9:56 AM

Document why we return EJUSTRETURN in do_execve().

This revision now requires review to proceed.Nov 21 2017, 10:06 AM
In D13180#274429, @kib wrote:

Note my comment at the return() in do_execve().

Yes! That sounds like a good idea.

As always, thanks for taking your time to review this change, Kostik. I'll commit it around the end of this week, so that people maintaining the platform bits still have some time to take a look at this.

I do not see changes for e.g. sparc64 where exec_setregs() also sets td_retval.

Remove dead code from sparc64. Spotted by kib@.

This revision is now accepted and ready to land.Nov 21 2017, 3:46 PM

We can also remove td_retval handling in the thread initialization.

This revision now requires review to proceed.Nov 21 2017, 3:47 PM
In D13180#274501, @kib wrote:

I do not see changes for e.g. sparc64 where exec_setregs() also sets td_retval.

Hey! Well spotted. In the case of sparc64, these pieces of code are somewhat indistinguishable from regular register preservation done in fetch_syscall_args(). I used:

vi $(grep -rl td_retval sys/{amd64,arm,arm64,i386,powerpc,riscv,sparc64,x86})

to get the source files that had to be altered. Let me double-check those.

Kostik, I see there are also some places where we set td_retval from within cpu_set_upcall(). That should be unnecessary altogether, right? Even without this change, we don't end up calling cpu_set_syscall_retval() after thread creation....

Shall I also go ahead and remove those assignments?

In D13180#274521, @ed wrote:

Kostik, I see there are also some places where we set td_retval from within cpu_set_upcall(). That should be unnecessary altogether, right? Even without this change, we don't end up calling cpu_set_syscall_retval() after thread creation....

Shall I also go ahead and remove those assignments?

Let's do it as a separate change. First push this one to svn, then I will review the follow-up.

Revert changes to cpu_set_upcall().

This will be fixed at a later time.

This revision was automatically updated to reflect the committed changes.