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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jJNZH4fQ6U197+A0nJeawHk+8yNnQj+mKft65L9o6_z7A@mail.gmail.com>
Date:	Sat, 27 Oct 2012 20:32:14 -0700
From:	Kees Cook <keescook@...omium.org>
To:	P J P <ppandit@...hat.com>
Cc:	Al Viro <viro@...iv.linux.org.uk>, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Josh Triplett <josh@...htriplett.org>,
	Serge Hallyn <serge.hallyn@...onical.com>,
	linux-fsdevel@...r.kernel.org, halfdog <me@...fdog.net>
Subject: Re: [PATCH] exec: do not leave bprm->interp on stack

On Sat, Oct 27, 2012 at 1:16 PM, P J P <ppandit@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/