From 814629dd64a35b829cc8f537b55b9c5d873f7bbf Mon Sep 17 00:00:00 2001 From: Ed Schouten Date: Fri, 24 Nov 2017 07:35:08 +0000 Subject: [PATCH] Don't let cpu_set_syscall_retval() clobber exec_setregs(). 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). 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. Reviewed by: kib, jhibbits Differential Revision: https://reviews.freebsd.org/D13180 --- sys/amd64/amd64/machdep.c | 1 - sys/amd64/ia32/ia32_signal.c | 1 - sys/amd64/linux32/linux32_sysvec.c | 1 - sys/arm/cloudabi32/cloudabi32_sysvec.c | 2 +- sys/arm64/arm64/machdep.c | 7 +------ sys/arm64/cloudabi64/cloudabi64_sysvec.c | 2 +- sys/compat/linux/linux_emul.c | 4 ++-- sys/i386/i386/machdep.c | 8 +------- sys/kern/init_main.c | 2 +- sys/kern/kern_exec.c | 10 ++++++++-- sys/powerpc/powerpc/exec_machdep.c | 13 ------------- sys/riscv/riscv/machdep.c | 7 +------ sys/sparc64/sparc64/machdep.c | 3 --- 13 files changed, 16 insertions(+), 45 deletions(-) diff --git a/sys/amd64/amd64/machdep.c b/sys/amd64/amd64/machdep.c index 3175616d0ce..0c16da1327d 100644 --- a/sys/amd64/amd64/machdep.c +++ b/sys/amd64/amd64/machdep.c @@ -604,7 +604,6 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) regs->tf_fs = _ufssel; regs->tf_gs = _ugssel; regs->tf_flags = TF_HASSEGS; - td->td_retval[1] = 0; /* * Reset the hardware debug registers if they were in use. diff --git a/sys/amd64/ia32/ia32_signal.c b/sys/amd64/ia32/ia32_signal.c index 79131f3f561..b1fc8debc1f 100644 --- a/sys/amd64/ia32/ia32_signal.c +++ b/sys/amd64/ia32/ia32_signal.c @@ -967,5 +967,4 @@ ia32_setregs(struct thread *td, struct image_params *imgp, u_long stack) /* Return via doreti so that we can change to a different %cs */ set_pcb_flags(pcb, PCB_32BIT | PCB_FULL_IRET); - td->td_retval[1] = 0; } diff --git a/sys/amd64/linux32/linux32_sysvec.c b/sys/amd64/linux32/linux32_sysvec.c index 8b0a35ea0bf..1a0dc1c954b 100644 --- a/sys/amd64/linux32/linux32_sysvec.c +++ b/sys/amd64/linux32/linux32_sysvec.c @@ -832,7 +832,6 @@ exec_linux_setregs(struct thread *td, struct image_params *imgp, u_long stack) /* Do full restore on return so that we can change to a different %cs */ set_pcb_flags(pcb, PCB_32BIT | PCB_FULL_IRET); - td->td_retval[1] = 0; } /* diff --git a/sys/arm/cloudabi32/cloudabi32_sysvec.c b/sys/arm/cloudabi32/cloudabi32_sysvec.c index a0bebcc9964..eeeb93c188c 100644 --- a/sys/arm/cloudabi32/cloudabi32_sysvec.c +++ b/sys/arm/cloudabi32/cloudabi32_sysvec.c @@ -61,7 +61,7 @@ cloudabi32_proc_setregs(struct thread *td, struct image_params *imgp, * tpidrurw to the TCB. */ regs = td->td_frame; - regs->tf_r0 = td->td_retval[0] = + regs->tf_r0 = stack + roundup(sizeof(cloudabi32_tcb_t), sizeof(register_t)); (void)cpu_set_user_tls(td, (void *)stack); } diff --git a/sys/arm64/arm64/machdep.c b/sys/arm64/arm64/machdep.c index 54eec90d726..452f224797a 100644 --- a/sys/arm64/arm64/machdep.c +++ b/sys/arm64/arm64/machdep.c @@ -311,12 +311,7 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) memset(tf, 0, sizeof(struct trapframe)); - /* - * We need to set x0 for init as it doesn't call - * cpu_set_syscall_retval to copy the value. We also - * need to set td_retval for the cases where we do. - */ - tf->tf_x[0] = td->td_retval[0] = stack; + tf->tf_x[0] = stack; tf->tf_sp = STACKALIGN(stack); tf->tf_lr = imgp->entry_addr; tf->tf_elr = imgp->entry_addr; diff --git a/sys/arm64/cloudabi64/cloudabi64_sysvec.c b/sys/arm64/cloudabi64/cloudabi64_sysvec.c index f6e75f56366..a98ff0e2c30 100644 --- a/sys/arm64/cloudabi64/cloudabi64_sysvec.c +++ b/sys/arm64/cloudabi64/cloudabi64_sysvec.c @@ -61,7 +61,7 @@ cloudabi64_proc_setregs(struct thread *td, struct image_params *imgp, * tpidr_el0 to the TCB. */ regs = td->td_frame; - regs->tf_x[0] = td->td_retval[0] = + regs->tf_x[0] = stack + roundup(sizeof(cloudabi64_tcb_t), sizeof(register_t)); (void)cpu_set_user_tls(td, (void *)stack); } diff --git a/sys/compat/linux/linux_emul.c b/sys/compat/linux/linux_emul.c index b244eea3b67..8be4fa2df0c 100644 --- a/sys/compat/linux/linux_emul.c +++ b/sys/compat/linux/linux_emul.c @@ -186,7 +186,7 @@ linux_common_execve(struct thread *td, struct image_args *eargs) error = kern_execve(td, eargs, NULL); post_execve(td, error, oldvmspace); - if (error != 0) + if (error != EJUSTRETURN) return (error); /* @@ -213,7 +213,7 @@ linux_common_execve(struct thread *td, struct image_args *eargs) free(em, M_TEMP); free(pem, M_LINUX); } - return (0); + return (EJUSTRETURN); } void diff --git a/sys/i386/i386/machdep.c b/sys/i386/i386/machdep.c index 68a45220858..dac5fd6967e 100644 --- a/sys/i386/i386/machdep.c +++ b/sys/i386/i386/machdep.c @@ -1126,6 +1126,7 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) set_fsbase(td, 0); set_gsbase(td, 0); + /* Make sure edx is 0x0 on entry. Linux binaries depend on it. */ bzero((char *)regs, sizeof(struct trapframe)); regs->tf_eip = imgp->entry_addr; regs->tf_esp = stack; @@ -1168,13 +1169,6 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) * clean FP state if it uses the FPU again. */ fpstate_drop(td); - - /* - * XXX - Linux emulator - * Make sure sure edx is 0x0 on entry. Linux binaries depend - * on it. - */ - td->td_retval[1] = 0; } void diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c index 645d120d980..dbd7a4ac07c 100644 --- a/sys/kern/init_main.c +++ b/sys/kern/init_main.c @@ -797,7 +797,7 @@ start_init(void *dummy) * Otherwise, return via fork_trampoline() all the way * to user mode as init! */ - if ((error = sys_execve(td, &args)) == 0) { + if ((error = sys_execve(td, &args)) == EJUSTRETURN) { mtx_unlock(&Giant); return; } diff --git a/sys/kern/kern_exec.c b/sys/kern/kern_exec.c index f844a61967c..284e08a801a 100644 --- a/sys/kern/kern_exec.c +++ b/sys/kern/kern_exec.c @@ -318,7 +318,7 @@ post_execve(struct thread *td, int error, struct vmspace *oldvmspace) * If success, we upgrade to SINGLE_EXIT state to * force other threads to suicide. */ - if (error == 0) + if (error == EJUSTRETURN) thread_single(p, SINGLE_EXIT); else thread_single_end(p, SINGLE_BOUNDARY); @@ -962,7 +962,13 @@ do_execve(struct thread *td, struct image_args *args, struct mac *mac_p) ktrprocctor(p); #endif - return (error); + /* + * We don't want cpu_set_syscall_retval() to overwrite any of + * the register values put in place by exec_setregs(). + * Implementations of cpu_set_syscall_retval() will leave + * registers unmodified when returning EJUSTRETURN. + */ + return (error == 0 ? EJUSTRETURN : error); } int diff --git a/sys/powerpc/powerpc/exec_machdep.c b/sys/powerpc/powerpc/exec_machdep.c index 596ae8079f1..724ae849a06 100644 --- a/sys/powerpc/powerpc/exec_machdep.c +++ b/sys/powerpc/powerpc/exec_machdep.c @@ -520,22 +520,11 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) * - ps_strings is a NetBSD extention, and will be * ignored by executables which are strictly * compliant with the SVR4 ABI. - * - * XXX We have to set both regs and retval here due to different - * XXX calling convention in trap.c and init_main.c. */ /* Collect argc from the user stack */ argc = fuword((void *)stack); - /* - * XXX PG: these get overwritten in the syscall return code. - * execve() should return EJUSTRETURN, like it does on NetBSD. - * Emulate by setting the syscall return value cells. The - * registers still have to be set for init's fork trampoline. - */ - td->td_retval[0] = argc; - td->td_retval[1] = stack + sizeof(register_t); tf->fixreg[3] = argc; tf->fixreg[4] = stack + sizeof(register_t); tf->fixreg[5] = stack + (2 + argc)*sizeof(register_t); @@ -572,8 +561,6 @@ ppc32_setregs(struct thread *td, struct image_params *imgp, u_long stack) argc = fuword32((void *)stack); - td->td_retval[0] = argc; - td->td_retval[1] = stack + sizeof(uint32_t); tf->fixreg[3] = argc; tf->fixreg[4] = stack + sizeof(uint32_t); tf->fixreg[5] = stack + (2 + argc)*sizeof(uint32_t); diff --git a/sys/riscv/riscv/machdep.c b/sys/riscv/riscv/machdep.c index d8a968791a0..de1a71e4d2e 100644 --- a/sys/riscv/riscv/machdep.c +++ b/sys/riscv/riscv/machdep.c @@ -279,12 +279,7 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) memset(tf, 0, sizeof(struct trapframe)); - /* - * We need to set a0 for init as it doesn't call - * cpu_set_syscall_retval to copy the value. We also - * need to set td_retval for the cases where we do. - */ - tf->tf_a[0] = td->td_retval[0] = stack; + tf->tf_a[0] = stack; tf->tf_sp = STACKALIGN(stack); tf->tf_ra = imgp->entry_addr; tf->tf_sepc = imgp->entry_addr; diff --git a/sys/sparc64/sparc64/machdep.c b/sys/sparc64/sparc64/machdep.c index 9d917afafbb..d450d436379 100644 --- a/sys/sparc64/sparc64/machdep.c +++ b/sys/sparc64/sparc64/machdep.c @@ -1009,9 +1009,6 @@ exec_setregs(struct thread *td, struct image_params *imgp, u_long stack) * header, it turns out that just always using TSO performs best. */ tf->tf_tstate = TSTATE_IE | TSTATE_PEF | TSTATE_MM_TSO; - - td->td_retval[0] = tf->tf_out[0]; - td->td_retval[1] = tf->tf_out[1]; } int