Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 19 May 2020 12:46:17 -0700
From:   Kees Cook <keescook@chromium.org>
To:     "Eric W. Biederman" <ebiederm@xmission.com>
Cc:     linux-kernel@vger.kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
        Greg Ungerer <gerg@linux-m68k.org>,
        Rob Landley <rob@landley.net>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Casey Schaufler <casey@schaufler-ca.com>,
        linux-security-module@vger.kernel.org,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Andy Lutomirski <luto@amacapital.net>
Subject: Re: [PATCH v2 7/8] exec: Generic execfd support
Message-ID: <202005191220.2DB7B7C7@keescook>
References: <87h7wujhmz.fsf@x220.int.ebiederm.org>
 <87sgga6ze4.fsf@x220.int.ebiederm.org>
 <87v9l4zyla.fsf_-_@x220.int.ebiederm.org>
 <877dx822er.fsf_-_@x220.int.ebiederm.org>
 <87y2poyd91.fsf_-_@x220.int.ebiederm.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87y2poyd91.fsf_-_@x220.int.ebiederm.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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