2015-04-04 13:59:35

by Denys Vlasenko

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

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

2015-04-04 14:04:23

by Ingo Molnar

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

* Denys Vlasenko <[email protected]> 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