Re: [PATCH] exec: do not leave bprm->interp on stack

[Kees Cook <keescook@chromium.org>]

On Sat, Oct 27, 2012 at 1:16 PM, P J P <ppandit@redhat.com> wrote:
> +-- On Sat, 27 Oct 2012, Kees Cook wrote --+
> | Al showed a list of them earlier in the thread.
>
>   Yeah, the list Al showed and I came across mostly has - binfmt_aout - entry.
> Do people still use - a.out - format? (considering ELF has been the default
> standard for so many years)

No one sane has CONFIG_BINFMT_AOUT any more. :)

> | I don't have any on the various distros I checked.
>
>   Same here, my F17 machine has no entries for binfmt-xxxx modules, in fact I
> don't even have the /etc/modprobe.d/aliases.conf file.
>
> Documentation/binfmt_misc.txt talks about executing Java, Python, DOSEMU and
> Windows programs which could be supported by loadable modules.

Right, but those are all registered from userspace and binfmt_misc
will catch them.

> | The problem I see here is that we only want to do module loading in the "no
> | match" case. But that means that either we need to restart with the original
> | bprm, or we need to keep bprm changes off the stack. Leading with a module
> | load is going to wreck performance.
>
>   I beg to *slightly* differ here. I agree we currently have a small overhead
> of find_module() -> request_module() only when binfmt_xxxx module is already
> loaded, partly because find_module can not resolve aliases.

Al's point here is that non of the binfmts are named "binfmt-NNNN".
Those are only aliases, and those are only visible from userspace,
even after they're loaded, so the find_module() call in your example
will never match anything. Which means if there is a non-printable in
a binary, the kernel will exec modprobe even if there is already a
binfmt that will load it.

> I guess this small overhead is worth it if it helps to make things less
> confusing and easy to follow. Besides this overhead does not exist for regular
> executables ELFs and scripts alike.
>
> If the required module is missing, a call to request_module() will anyway
> happen and its cost remains the same whether it happens before or after the
> "match".

Well, we'll always do the modprobe call-out on a missing binfmt (so
for that reason I like the addition of the printk, though it should
probably be rate-limited), but we still need to only load modules at
fall-back time. Which means means we need to return from the recursive
loading attempt, which means we need to restart the bprm (slow) or we
need to do a heap alloc for the changed interp (less slow).

If we change binfmt_script to not make a recursive call, then we still
need to keep the interp change somewhere off the stack. I still think
my patchset is the least bad.

Al, do you have something else in mind?

-Kees

-- 
Kees Cook
Chrome OS Security