[v2,0/8] exec: Control flow simplifications
mbox series

Message ID 877dx822er.fsf_-_@x220.int.ebiederm.org
Headers show
Series
  • exec: Control flow simplifications
Related show

Message

Eric W. Biederman May 19, 2020, 12:29 a.m. UTC
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(-)

Comments

Linus Torvalds May 19, 2020, 1:25 a.m. UTC | #1
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
Kees Cook May 19, 2020, 9:55 p.m. UTC | #2
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
Eric W. Biederman May 20, 2020, 1:02 p.m. UTC | #3
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
Eric W. Biederman May 20, 2020, 10:12 p.m. UTC | #4
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 ##
Kees Cook May 20, 2020, 11:43 p.m. UTC | #5
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
Eric W. Biederman May 21, 2020, 11:53 a.m. UTC | #6
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
Eric W. Biederman May 28, 2020, 3:38 p.m. UTC | #7
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