Re: [PATCH v2 7/8] exec: Generic execfd support

[Kees Cook <keescook@chromium.org>]

On Mon, May 18, 2020 at 07:33:46PM -0500, Eric W. Biederman wrote:
> 
> Most of the support for passing the file descriptor of an executable
> to an interpreter already lives in the generic code and in binfmt_elf.
> Rework the fields in binfmt_elf that deal with executable file
> descriptor passing to make executable file descriptor passing a first
> class concept.
> 
> Move the fd_install from binfmt_misc into begin_new_exec after the new
> creds have been installed.  This means that accessing the file through
> /proc/<pid>/fd/N is able to see the creds for the new executable
> before allowing access to the new executables files.
> 
> Performing the install of the executables file descriptor after
> the point of no return also means that nothing special needs to
> be done on error.  The exiting of the process will close all
> of it's open files.
> 
> Move the would_dump from binfmt_misc into begin_new_exec right
> after would_dump is called on the bprm->file.  This makes it
> obvious this case exists and that no nesting of bprm->file is
> currently supported.
> 
> In binfmt_misc the movement of fd_install into generic code means
> that it's special error exit path is no longer needed.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

Yes, this is so much nicer. :) My head did spin a little between changing
the management of bprm->executable between this patch and the next,
but I'm okay now. ;)

Reviewed-by: Kees Cook <keescook@chromium.org>

nits/thoughts below...

> [...]
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index 8c7779d6bf19..653508b25815 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> [...]
> @@ -48,6 +51,7 @@ struct linux_binprm {
>  	unsigned int taso:1;
>  #endif
>  	unsigned int recursion_depth; /* only for search_binary_handler() */
> +	struct file * executable; /* Executable to pass to the interpreter */
>  	struct file * file;
>  	struct cred *cred;	/* new credentials */

nit: can we fix the "* " stuff here? This should be *file and *executable.

> [...]
> @@ -69,10 +73,6 @@ struct linux_binprm {
>  #define BINPRM_FLAGS_ENFORCE_NONDUMP_BIT 0
>  #define BINPRM_FLAGS_ENFORCE_NONDUMP (1 << BINPRM_FLAGS_ENFORCE_NONDUMP_BIT)
>  
> -/* fd of the binary should be passed to the interpreter */
> -#define BINPRM_FLAGS_EXECFD_BIT 1
> -#define BINPRM_FLAGS_EXECFD (1 << BINPRM_FLAGS_EXECFD_BIT)
> -
>  /* filename of the binary will be inaccessible after exec */
>  #define BINPRM_FLAGS_PATH_INACCESSIBLE_BIT 2
>  #define BINPRM_FLAGS_PATH_INACCESSIBLE (1 << BINPRM_FLAGS_PATH_INACCESSIBLE_BIT)

nit: may as well renumber BINPRM_FLAGS_PATH_INACCESSIBLE_BIT to 1,
they're not UAPI. And, actually, nothing uses the *_BIT defines, so
probably the entire chunk of code could just be reduced to:

/* either interpreter or executable was unreadable */
#define BINPRM_FLAGS_ENFORCE_NONDUMP    BIT(0)
/* filename of the binary will be inaccessible after exec */
#define BINPRM_FLAGS_PATH_INACCESSIBLE  BIT(1)

Though frankly, I wonder if interp_flags could just be removed in favor
of two new bit members, especially since interp_data is gone:

+               /* Either interpreter or executable was unreadable. */
+               nondumpable:1;
+               /* Filename of the binary will be inaccessible after exec. */
+               path_inaccessible:1;
...
-       unsigned interp_flags;
...etc

-- 
Kees Cook