| Message ID | 877dx822er.fsf_-_@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | exec: Control flow simplifications | expand |
On Mon, May 18, 2020 at 5:32 PM Eric W. Biederman <ebiederm@xmission.com> wrote: > > It is hard to follow the control flow in exec.c as the code has evolved over > time and something that used to work one way now works another. This set of > changes attempts to address the worst of that, to remove unnecessary work > and to make the code a little easier to follow. It is indeed hard to follow, and maybe I missed something, but from what I can tell, your series looks all sane. It certainly seems to make things much more straightforward. Of course, exactly _because_ it's such a messy area, maybe it introduces something odd, but all the patches look relatively straightforward. And you remove more lines of code than you add, which is always nice to see. So ack from me. Oleg? Jann? Anybody? Do you see anything strange that I missed? Linus
On Mon, May 18, 2020 at 07:29:00PM -0500, Eric W. Biederman wrote: > arch/alpha/kernel/binfmt_loader.c | 11 +---- > fs/binfmt_elf.c | 4 +- > fs/binfmt_elf_fdpic.c | 4 +- > fs/binfmt_em86.c | 13 +---- > fs/binfmt_misc.c | 69 ++++----------------------- > fs/binfmt_script.c | 82 ++++++++++++++------------------ > fs/exec.c | 97 ++++++++++++++++++++++++++------------ > include/linux/binfmts.h | 36 ++++++-------- > include/linux/lsm_hook_defs.h | 3 +- > include/linux/lsm_hooks.h | 52 +++++++++++--------- > include/linux/security.h | 14 ++++-- > kernel/cred.c | 3 ++ > security/apparmor/domain.c | 7 +-- > security/apparmor/include/domain.h | 2 +- > security/apparmor/lsm.c | 2 +- > security/commoncap.c | 9 ++-- > security/security.c | 9 +++- > security/selinux/hooks.c | 8 ++-- > security/smack/smack_lsm.c | 9 ++-- > security/tomoyo/tomoyo.c | 12 ++--- > 20 files changed, 202 insertions(+), 244 deletions(-) Oh, BTW, heads up on this (trivially but annoyingly) conflicting with the copy_strings_kernel/copy_string/kernel change: https://ozlabs.org/~akpm/mmotm/broken-out/exec-simplify-the-copy_strings_kernel-calling-convention.patch Is it worth pulling that and these into your tree? https://ozlabs.org/~akpm/mmotm/broken-out/exec-open-code-copy_string_kernel.patch https://ozlabs.org/~akpm/mmotm/broken-out/umh-fix-refcount-underflow-in-fork_usermode_blob.patch
Kees Cook <keescook@chromium.org> writes: > On Mon, May 18, 2020 at 07:29:00PM -0500, Eric W. Biederman wrote: >> arch/alpha/kernel/binfmt_loader.c | 11 +---- >> fs/binfmt_elf.c | 4 +- >> fs/binfmt_elf_fdpic.c | 4 +- >> fs/binfmt_em86.c | 13 +---- >> fs/binfmt_misc.c | 69 ++++----------------------- >> fs/binfmt_script.c | 82 ++++++++++++++------------------ >> fs/exec.c | 97 ++++++++++++++++++++++++++------------ >> include/linux/binfmts.h | 36 ++++++-------- >> include/linux/lsm_hook_defs.h | 3 +- >> include/linux/lsm_hooks.h | 52 +++++++++++--------- >> include/linux/security.h | 14 ++++-- >> kernel/cred.c | 3 ++ >> security/apparmor/domain.c | 7 +-- >> security/apparmor/include/domain.h | 2 +- >> security/apparmor/lsm.c | 2 +- >> security/commoncap.c | 9 ++-- >> security/security.c | 9 +++- >> security/selinux/hooks.c | 8 ++-- >> security/smack/smack_lsm.c | 9 ++-- >> security/tomoyo/tomoyo.c | 12 ++--- >> 20 files changed, 202 insertions(+), 244 deletions(-) > > Oh, BTW, heads up on this (trivially but annoyingly) conflicting with > the copy_strings_kernel/copy_string/kernel change: > > https://ozlabs.org/~akpm/mmotm/broken-out/exec-simplify-the-copy_strings_kernel-calling-convention.patch > > Is it worth pulling that and these into your tree? > > https://ozlabs.org/~akpm/mmotm/broken-out/exec-open-code-copy_string_kernel.patch > > https://ozlabs.org/~akpm/mmotm/broken-out/umh-fix-refcount-underflow-in-fork_usermode_blob.patch Good question. It is part of the greater set_fs removal work, and I don't want to mess that up. I would love to give copy_string_kernel a length parameter so binfmt_script did not have to modify it's buffer or copy the string, before calling copy_string_kernel. Hmm. I already have to call strdup on i_name in brpm_change_interp. So I probably just want to bite the bullet and figure out a way to do strdup earlier. So unless it makes things easier for Andrew I think it is probably easier to live with the conflict for now, and use this conversation as inspiration for my next round of cleanups of binfmt_misc. Eric
I have pushed this out to:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next
I have collected up the acks and reviewed-by's, and fixed a couple of
typos but that is it.
If we need comment fixes or additional cleanups we can apply that on top
of this series. This way the code can sit in linux-next until the
merge window opens.
Before I pushed this out I also tested this with Kees new test of
binfmt_misc and did not find any problems.
Eric
The git range-diff of the changes I applied before pushing this out:
1: f6bb0d6563ca ! 1: 87b047d2be41 exec: Teach prepare_exec_creds how exec treats uids & gids
@@ Commit message
update bprm->cred are just need to handle special cases such
as setuid exec and change of domains.
+ Link: https://lkml.kernel.org/r/871rng22dm.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## kernel/cred.c ##
2: d3b3594be22f ! 2: b8bff599261c exec: Factor security_bprm_creds_for_exec out of security_bprm_set_creds
@@ Commit message
Add or upate comments a appropriate to bring them up to date and
to reflect this change.
+ Link: https://lkml.kernel.org/r/87v9kszrzh.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Acked-by: Casey Schaufler <casey@schaufler-ca.com> # For the LSM and Smack bits
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/exec.c ##
3: 65c651a77967 ! 3: d9d67b76eed6 exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds
@@ Commit message
In short two renames and a move in the location of initializing
bprm->active_secureexec.
+ Link: https://lkml.kernel.org/r/87o8qkzrxp.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/exec.c ##
4: 6d0d5da2b45e ! 4: dbf17e846ea9 exec: Allow load_misc_binary to call prepare_binfmt unconditionally
@@ Metadata
Author: Eric W. Biederman <ebiederm@xmission.com>
## Commit message ##
- exec: Allow load_misc_binary to call prepare_binfmt unconditionally
+ exec: Allow load_misc_binary to call prepare_binprm unconditionally
Add a flag preserve_creds that binfmt_misc can set to prevent
credentials from being updated. This allows binfmt_misc to always
- call prepare_binfmt. Allowing the credential computation logic to be
+ call prepare_binprm. Allowing the credential computation logic to be
consolidated.
Not replacing the credentials with the interpreters credentials is
@@ Commit message
exec sees.
Ref: c407c033de84 ("[PATCH] binfmt_misc: improve calculation of interpreter's credentials")
+ Link: https://lkml.kernel.org/r/87imgszrwo.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/binfmt_misc.c ##
5: af7db65c2483 ! 5: 8a8f3bb8ec41 exec: Move the call of prepare_binprm into search_binary_handler
@@ Commit message
search_binary_handler is called so move the call into search_binary_handler
itself to make the code simpler and easier to understand.
+ Link: https://lkml.kernel.org/r/87d070zrvx.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
+ Reviewed-by: James Morris <jamorris@linux.microsoft.com>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## arch/alpha/kernel/binfmt_loader.c ##
6: 69fccdf33a87 ! 6: 01dbc34d75bf exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC
@@ Commit message
has been take that the logic of the parsing code (short of replacing
characters by '\0') remains the same.
+ Link: https://lkml.kernel.org/r/874ksczru6.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/binfmt_script.c ##
7: 30fe957c6dce ! 7: 6962a6b4de92 exec: Generic execfd support
@@ Commit message
In binfmt_misc the movement of fd_install into generic code means
that it's special error exit path is no longer needed.
+ Link: https://lkml.kernel.org/r/87y2poyd91.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## fs/binfmt_elf.c ##
@@ fs/exec.c: int begin_new_exec(struct linux_binprm * bprm)
*/
set_mm_exe_file(bprm->mm, bprm->file);
-+ /* If the binary is not readable than enforce mm->dumpable=0 */
++ /* If the binary is not readable then enforce mm->dumpable=0 */
would_dump(bprm, bprm->file);
+ if (bprm->have_execfd)
+ would_dump(bprm, bprm->executable);
8: f0a27d0fde69 ! 8: 226ce5863881 exec: Remove recursion from search_binary_handler
@@ Commit message
reassignments of bprm->file moved to exec_binprm bprm->file can never
be NULL in search_binary_handler.
+ Link: https://lkml.kernel.org/r/87sgfwyd84.fsf_-_@x220.int.ebiederm.org
+ Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
+ Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
## arch/alpha/kernel/binfmt_loader.c ##
On Wed, May 20, 2020 at 05:12:10PM -0500, Eric W. Biederman wrote: > > I have pushed this out to: > > git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next > > I have collected up the acks and reviewed-by's, and fixed a couple of > typos but that is it. Awesome! > If we need comment fixes or additional cleanups we can apply that on top > of this series. This way the code can sit in linux-next until the > merge window opens. > > Before I pushed this out I also tested this with Kees new test of > binfmt_misc and did not find any problems. Did this mean to say binfmt_script? It'd be nice to get a binfmt_misc test too, though. Thanks! -Kees
Kees Cook <keescook@chromium.org> writes: > On Wed, May 20, 2020 at 05:12:10PM -0500, Eric W. Biederman wrote: >> >> I have pushed this out to: >> >> git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next >> >> I have collected up the acks and reviewed-by's, and fixed a couple of >> typos but that is it. > > Awesome! > >> If we need comment fixes or additional cleanups we can apply that on top >> of this series. This way the code can sit in linux-next until the >> merge window opens. >> >> Before I pushed this out I also tested this with Kees new test of >> binfmt_misc and did not find any problems. > > Did this mean to say binfmt_script? It'd be nice to get a binfmt_misc > test too, though. Yes. Sorry. I meant your binfmt_script test. Eric
Recomputing the uids, gids, capabilities, and related flags each time a
new bprm->file is set is error prone, and as it turns out unnecessary.
Further our decisions on when to clear personality bits and when to tell
userspace privileges have been gained so please be extra careful, is
imperfect and our current code overshoots in inconsistent ways making
it hard to understand what is happening, and why.
Building upon my previous exec clean up work this set of changes moves
the bprm->cred calculations a little later so they only need to be done
once, moves all of the uid and gid handling into bprm_fill_uid, and
then cleans up setting secureexec and per_clear so they happen when they
make sense from a semantic perspective.
One of the largest challenges is dealing with how we revert the
credential change if it is discovered the process calling exec is
ptraced and the tracer does not have enough credentials. It looks
like that code was tacked on as an after thought to a bug fix that
went into 2.4.0-prerelease.
I don't know if we have ever gotten all of the details just right when
the credentials are rolled back. So this set of changes causes the
credentials not to be changed when ptraced, instead of attempting to
rollback the credential change.
Folks please give this code a review and let me know if you see
anything.
Eric W. Biederman (11):
exec: Reduce bprm->per_clear to a single bit
exec: Introduce active_per_clear the per file version of per_clear
exec: Compute file based creds only once
exec: Move uid/gid handling from creds_from_file into bprm_fill_uid
exec: In bprm_fill_uid use CAP_SETGID to see if a gid change is safe
exec: Don't set secureexec when the uid or gid changes are abandoned
exec: Set saved, fs, and effective ids together in bprm_fill_uid
exec: In bprm_fill_uid remove unnecessary no new privs check
exec: In bprm_fill_uid only set per_clear when honoring suid or sgid
exec: In bprm_fill_uid set secureexec at same time as per_clear
exec: Remove the label after_setid from bprm_fill_uid
fs/binfmt_misc.c | 2 +-
fs/exec.c | 95 +++++++++++++++++++++++++------------------
include/linux/binfmts.h | 13 +++---
include/linux/lsm_hook_defs.h | 2 +-
include/linux/lsm_hooks.h | 21 ++++++----
include/linux/security.h | 8 ++--
security/apparmor/domain.c | 2 +-
security/commoncap.c | 37 ++++++-----------
security/security.c | 4 +-
security/selinux/hooks.c | 2 +-
security/smack/smack_lsm.c | 2 +-
11 files changed, 98 insertions(+), 90 deletions(-)
---
This builds upon my previous exec cleanup work at:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exec-next
Thank you,
Eric
It is hard to follow the control flow in exec.c as the code has evolved over time and something that used to work one way now works another. This set of changes attempts to address the worst of that, to remove unnecessary work and to make the code a little easier to follow. The churn is a bit higher than the last version of this patchset, with renaming and cleaning up of comments. I have split security_bprm_set_creds into security_bprm_creds_for_exec and security_bprm_repopulate_creds. My goal was to make it clear that one hook completes its work while the other recaculates it's work each time a new interpreter is selected. I have added a new change at the beginning to make it clear that neither security_bprm_creds_for_exec nor security_bprm_repopulate_creds needs to be implemented as prepare_exec_creds properly does the work of setting up credentials unless something special is going on. I have made the execfd support generic and moved out of binfmt_misc so that I can remove the recursion. I have moved reassigning bprm->file into the loop that replaces the recursion. In doing so I discovered that binfmt_misc was naughty and was returning -ENOEXEC in such a way that the search_binary_handler loop could not continue. So I added a change to remove that naughtiness. Eric W. Biederman (8): exec: Teach prepare_exec_creds how exec treats uids & gids exec: Factor security_bprm_creds_for_exec out of security_bprm_set_creds exec: Convert security_bprm_set_creds into security_bprm_repopulate_creds exec: Allow load_misc_binary to call prepare_binfmt unconditionally exec: Move the call of prepare_binprm into search_binary_handler exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC exec: Generic execfd support exec: Remove recursion from search_binary_handler arch/alpha/kernel/binfmt_loader.c | 11 +---- fs/binfmt_elf.c | 4 +- fs/binfmt_elf_fdpic.c | 4 +- fs/binfmt_em86.c | 13 +---- fs/binfmt_misc.c | 69 ++++----------------------- fs/binfmt_script.c | 82 ++++++++++++++------------------ fs/exec.c | 97 ++++++++++++++++++++++++++------------ include/linux/binfmts.h | 36 ++++++-------- include/linux/lsm_hook_defs.h | 3 +- include/linux/lsm_hooks.h | 52 +++++++++++--------- include/linux/security.h | 14 ++++-- kernel/cred.c | 3 ++ security/apparmor/domain.c | 7 +-- security/apparmor/include/domain.h | 2 +- security/apparmor/lsm.c | 2 +- security/commoncap.c | 9 ++-- security/security.c | 9 +++- security/selinux/hooks.c | 8 ++-- security/smack/smack_lsm.c | 9 ++-- security/tomoyo/tomoyo.c | 12 ++--- 20 files changed, 202 insertions(+), 244 deletions(-)