Commit 9d9488d4 authored by Eric W. Biederman's avatar Eric W. Biederman
Browse files

exec: Control flow simplifications

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(-)

Link: https://lkml.kernel.org/r/877dx822er.fsf_-_@x220.int.ebiederm.org


Acked-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: default avatar"Eric W. Biederman" <ebiederm@xmission.com>
parents b127c16d bc2bf338
Loading
Loading
Loading
Loading
+2 −9
Original line number Diff line number Diff line
@@ -19,10 +19,6 @@ static int load_binary(struct linux_binprm *bprm)
	if (bprm->loader)
		return -ENOEXEC;

	allow_write_access(bprm->file);
	fput(bprm->file);
	bprm->file = NULL;

	loader = bprm->vma->vm_end - sizeof(void *);

	file = open_exec("/sbin/loader");
@@ -33,12 +29,9 @@ static int load_binary(struct linux_binprm *bprm)
	/* Remember if the application is TASO.  */
	bprm->taso = eh->ah.entry < 0x100000000UL;

	bprm->file = file;
	bprm->interpreter = file;
	bprm->loader = loader;
	retval = prepare_binprm(bprm);
	if (retval < 0)
		return retval;
	return search_binary_handler(bprm);
	return 0;
}

static struct linux_binfmt loader_format = {
+2 −2
Original line number Diff line number Diff line
@@ -273,8 +273,8 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
		NEW_AUX_ENT(AT_BASE_PLATFORM,
			    (elf_addr_t)(unsigned long)u_base_platform);
	}
	if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) {
		NEW_AUX_ENT(AT_EXECFD, bprm->interp_data);
	if (bprm->have_execfd) {
		NEW_AUX_ENT(AT_EXECFD, bprm->execfd);
	}
#undef NEW_AUX_ENT
	/* AT_NULL is zero; clear the rest too */
+2 −2
Original line number Diff line number Diff line
@@ -628,10 +628,10 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm,
			    (elf_addr_t) (unsigned long) u_base_platform);
	}

	if (bprm->interp_flags & BINPRM_FLAGS_EXECFD) {
	if (bprm->have_execfd) {
		nr = 0;
		csp -= 2 * sizeof(unsigned long);
		NEW_AUX_ENT(AT_EXECFD, bprm->interp_data);
		NEW_AUX_ENT(AT_EXECFD, bprm->execfd);
	}

	nr = 0;
+2 −11
Original line number Diff line number Diff line
@@ -48,10 +48,6 @@ static int load_em86(struct linux_binprm *bprm)
	if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
		return -ENOENT;

	allow_write_access(bprm->file);
	fput(bprm->file);
	bprm->file = NULL;

	/* Unlike in the script case, we don't have to do any hairy
	 * parsing to find our interpreter... it's hardcoded!
	 */
@@ -89,13 +85,8 @@ static int load_em86(struct linux_binprm *bprm)
	if (IS_ERR(file))
		return PTR_ERR(file);

	bprm->file = file;

	retval = prepare_binprm(bprm);
	if (retval < 0)
		return retval;

	return search_binary_handler(bprm);
	bprm->interpreter = file;
	return 0;
}

static struct linux_binfmt em86_format = {
+10 −59
Original line number Diff line number Diff line
@@ -134,7 +134,6 @@ static int load_misc_binary(struct linux_binprm *bprm)
	Node *fmt;
	struct file *interp_file = NULL;
	int retval;
	int fd_binary = -1;

	retval = -ENOEXEC;
	if (!enabled)
@@ -160,51 +159,25 @@ static int load_misc_binary(struct linux_binprm *bprm)
			goto ret;
	}

	if (fmt->flags & MISC_FMT_OPEN_BINARY) {
	if (fmt->flags & MISC_FMT_OPEN_BINARY)
		bprm->have_execfd = 1;

		/* if the binary should be opened on behalf of the
		 * interpreter than keep it open and assign descriptor
		 * to it
		 */
		fd_binary = get_unused_fd_flags(0);
		if (fd_binary < 0) {
			retval = fd_binary;
			goto ret;
		}
		fd_install(fd_binary, bprm->file);

		/* if the binary is not readable than enforce mm->dumpable=0
		   regardless of the interpreter's permissions */
		would_dump(bprm, bprm->file);

		allow_write_access(bprm->file);
		bprm->file = NULL;

		/* mark the bprm that fd should be passed to interp */
		bprm->interp_flags |= BINPRM_FLAGS_EXECFD;
		bprm->interp_data = fd_binary;

	} else {
		allow_write_access(bprm->file);
		fput(bprm->file);
		bprm->file = NULL;
	}
	/* make argv[1] be the path to the binary */
	retval = copy_strings_kernel(1, &bprm->interp, bprm);
	if (retval < 0)
		goto error;
		goto ret;
	bprm->argc++;

	/* add the interp as argv[0] */
	retval = copy_strings_kernel(1, &fmt->interpreter, bprm);
	if (retval < 0)
		goto error;
		goto ret;
	bprm->argc++;

	/* Update interp in case binfmt_script needs it. */
	retval = bprm_change_interp(fmt->interpreter, bprm);
	if (retval < 0)
		goto error;
		goto ret;

	if (fmt->flags & MISC_FMT_OPEN_FILE) {
		interp_file = file_clone_open(fmt->interp_file);
@@ -215,38 +188,16 @@ static int load_misc_binary(struct linux_binprm *bprm)
	}
	retval = PTR_ERR(interp_file);
	if (IS_ERR(interp_file))
		goto error;

	bprm->file = interp_file;
	if (fmt->flags & MISC_FMT_CREDENTIALS) {
		loff_t pos = 0;

		/*
		 * No need to call prepare_binprm(), it's already been
		 * done.  bprm->buf is stale, update from interp_file.
		 */
		memset(bprm->buf, 0, BINPRM_BUF_SIZE);
		retval = kernel_read(bprm->file, bprm->buf, BINPRM_BUF_SIZE,
				&pos);
	} else
		retval = prepare_binprm(bprm);
		goto ret;

	if (retval < 0)
		goto error;

	retval = search_binary_handler(bprm);
	if (retval < 0)
		goto error;
	bprm->interpreter = interp_file;
	if (fmt->flags & MISC_FMT_CREDENTIALS)
		bprm->preserve_creds = 1;

	retval = 0;
ret:
	dput(fmt->dentry);
	return retval;
error:
	if (fd_binary > 0)
		ksys_close(fd_binary);
	bprm->interp_flags = 0;
	bprm->interp_data = 0;
	goto ret;
}

/* Command parsers */
Loading