lkml.org 
[lkml]   [2020]   [May]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 7/8] exec: Generic execfd support
    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

    \
     
     \ /
      Last update: 2020-05-19 21:46    [W:4.059 / U:0.260 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site