sys_execve leaking rbp/rbx/r12-15 to the new process?

sys_execve leaking rbp/rbx/r12-15 to the new process?

[Denys Vlasenko]

Hi guys,

I was looking at some optimizations in stub_execve.

In particular, not doing SAVE_EXTRA_REGS. (To recap,
SAVE_EXTRA_REGS populates pt_regs->bp/bx/r12-15).


It seems redundant, because sys_execve overwrites them via:

static int load_elf_binary(struct linux_binprm *bprm)
{
...
#ifdef ELF_PLAT_INIT
        /*
         * The ABI may specify that certain registers be set up in special
         * ways (on i386 %edx is the address of a DT_FINI function, for
         * example.  In addition, it may also specify (eg, PowerPC64 ELF)
         * that the e_entry field is the address of the function descriptor
         * for the startup routine, rather than the address of the startup
         * routine itself.  This macro performs whatever initialization to
         * the regs structure is required as well as any relocations to the
         * function descriptor entries when executing dynamically links apps.
         */
        ELF_PLAT_INIT(regs, reloc_func_desc);
#endif


where ELF_PLAT_INIT is:

#define ELF_PLAT_INIT(_r, load_addr)                    \
        elf_common_init(&current->thread, _r, 0)

static inline void elf_common_init(struct thread_struct *t,
                                   struct pt_regs *regs, const u16 ds)
{
        regs->ax = regs->bx = regs->cx = regs->dx = 0;
        regs->si = regs->di = regs->bp = 0;
        regs->r8 = regs->r9 = regs->r10 = regs->r11 = 0;
        regs->r12 = regs->r13 = regs->r14 = regs->r15 = 0;
        t->fs = t->gs = 0;
        t->fsindex = t->gsindex = 0;
        t->ds = t->es = ds;
}

But then I recalled than ELF is not all that is. Ho hum.

binfmt_flat.c has similar FLAT_PLAT_INIT, but x86 (and everyone else
except sh) doesn't define it.

binfmt_elf_fdpic.c has ELF_FDPIC_PLAT_INIT, but x86 (and most others)
doesn't define it.

I don't see any such hooks in binfmt_aout.c et al.

IOW: it looks like we do not clear these registers for any executable
types except standard ELF. We inherit their values from the prior executable.

Is this intended?

I'm asking because this inheriting of registers is not "free", we actively
make it happen:

ENTRY(stub_execve)
        CFI_STARTPROC
        addq $8, %rsp
        SAVE_EXTRA_REGS                 <====
        call sys_execve
        movq %rax,RAX(%rsp)
        RESTORE_EXTRA_REGS              <====
        jmp int_ret_from_sys_call
        CFI_ENDPROC
END(stub_execve)

which is kinda stupid if we don't actually want this to happen,
if we instead want them zeroed on success, and unchanged on failure.
Those two macros expand into twelve 5-byte instructions.

We can do this instead:

ENTRY(stub_execve)
        CFI_STARTPROC
        call	sys_execve
	testl	%eax, %eax
	jz	1f
	ret
1:      addq	$8, %rsp
	xorl	%ebx, %ebx    // maybe create a macro for zeroing these
	xorl	%ebp, %ebp    //
	xorl	%r12d, %r12d  //
	xorl	%r13d, %r13d  //
	xorl	%r14d, %r14d  //
	xorl	%r15d, %r15d  //
        movq	%rax,RAX(%rsp)  /* zero */
        jmp	int_ret_from_sys_call
        CFI_ENDPROC
END(stub_execve)

The elf_common_init() does not need to zero regs->ax/bx/bp/r12-15 anymore,
it's done by the code above.


This achieves the following:

* all executable types, not just ELF, get these regs zeroed
* error returns are faster (don't use IRET return code path)
* stores to stack are gone
* XOR's are much faster than loads from stack (and smaller too)

Any reason we should not do this change?

-- 
vda

Re: sys_execve leaking rbp/rbx/r12-15 to the new process?

[Ingo Molnar]

* Denys Vlasenko <dvlasenk@redhat.com> wrote:

> We can do this instead:
> 
> ENTRY(stub_execve)
>         CFI_STARTPROC
>         call	sys_execve
> 	testl	%eax, %eax
> 	jz	1f
> 	ret
> 1:      addq	$8, %rsp
> 	xorl	%ebx, %ebx    // maybe create a macro for zeroing these
> 	xorl	%ebp, %ebp    //
> 	xorl	%r12d, %r12d  //
> 	xorl	%r13d, %r13d  //
> 	xorl	%r14d, %r14d  //
> 	xorl	%r15d, %r15d  //

Yeah, call it ZERO_EXTRA_REGS or so.

> Any reason we should not do this change?

Not that I can see - lets try it.

Thanks,

	Ingo