From: Chris Wright This patchset looks OK, except for one problem. It installs the fd (which could've been unreadable) without unsharing the ->files. So someone can use this to read unreadable yet executable files. Here's a patch which fixes that up. I added one bit that's commented out because I'm not positive if a final steal_locks() is needed. I did a fair amount of rearranging to simplify the error conditions relative to the fd_install(), and unshare_files(). I can work on the last steal_locks() bit later tonight or tmomorrow. I've actually tested all cases they added, plus the fd_install race and my fix (all are good), but the intel folks should take this patch for a whirl too. --- 25-akpm/fs/binfmt_misc.c | 116 +++++++++++++++++++++-------------------------- 1 files changed, 54 insertions(+), 62 deletions(-) diff -puN fs/binfmt_misc.c~binfmt_misc-credentials-fixes fs/binfmt_misc.c --- 25/fs/binfmt_misc.c~binfmt_misc-credentials-fixes 2004-04-29 22:53:45.621902752 -0700 +++ 25-akpm/fs/binfmt_misc.c 2004-04-29 22:53:45.625902144 -0700 @@ -105,14 +105,12 @@ static int load_misc_binary(struct linux { Node *fmt; struct file * interp_file = NULL; - struct file * binary_file = NULL; char iname[BINPRM_BUF_SIZE]; char *iname_addr = iname; int retval; int fd_binary = -1; - char fd_str[32]; - char * fdsp = fd_str; - int is_open_bin; + char fd_str[12]; + struct files_struct *files = NULL; retval = -ENOEXEC; if (!enabled) @@ -127,39 +125,52 @@ static int load_misc_binary(struct linux if (!fmt) goto _ret; - is_open_bin = (fmt->flags & MISC_FMT_OPEN_BINARY) ? 1 : 0; + if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) { + remove_arg_zero(bprm); + } - if (is_open_bin) { + if (fmt->flags & MISC_FMT_OPEN_BINARY) { + char *fdsp = fd_str; + + files = current->files; + retval = unshare_files(); + if (retval < 0) + goto _ret; + if (files == current->files) { + put_files_struct(files); + files = NULL; + } /* 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 (); + fd_binary = get_unused_fd(); if (fd_binary < 0) { retval = fd_binary; - goto _ret; + goto _unshare; } - snprintf (fd_str, sizeof(fd_str) - 1, "%d", fd_binary); - } else { - allow_write_access (bprm->file); - fput (bprm->file); - bprm->file = NULL; - } - - /* Build args for interpreter */ - if (!(fmt->flags & MISC_FMT_PRESERVE_ARGV0)) { - remove_arg_zero(bprm); - } + fd_install(fd_binary, bprm->file); - if (is_open_bin) { /* make argv[1] be the file descriptor of the binary */ - retval = copy_strings_kernel (1, &fdsp, bprm); + snprintf(fd_str, sizeof(fd_str), "%d", fd_binary); + retval = copy_strings_kernel(1, &fdsp, bprm); + if (retval < 0) + goto _error; + bprm->argc++; + + /* if the binary is not readable than enforce mm->dumpable=0 + regardless of the interpreter's permissions */ + if (permission(bprm->file->f_dentry->d_inode, MAY_READ, NULL)) + bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP; } 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; + bprm->argc++; } - if (retval < 0) - goto _error; - bprm->argc ++; retval = copy_strings_kernel (1, &iname_addr, bprm); if (retval < 0) goto _error; @@ -171,61 +182,42 @@ static int load_misc_binary(struct linux if (IS_ERR (interp_file)) goto _error; - - binary_file = bprm->file; + bprm->file = interp_file; if (fmt->flags & MISC_FMT_CREDENTIALS) { /* - * Call prepare_binprm before switching to interpreter's file - * so that all security calculation will be done according to - * binary and not interpreter + * No need to call prepare_binprm(), it's already been + * done. bprm->buf is stale, update from interp_file. */ - retval = prepare_binprm(bprm); - if (retval < 0) - goto _error; - bprm->file = interp_file; memset(bprm->buf, 0, BINPRM_BUF_SIZE); retval = kernel_read(bprm->file, 0, bprm->buf, BINPRM_BUF_SIZE); - } else { - bprm->file = interp_file; + } else retval = prepare_binprm (bprm); - } if (retval < 0) goto _error; - if (is_open_bin) { - /* if the binary is not readable than enforce mm->dumpable=0 - regardless of the interpreter's permissions */ - if (permission (binary_file->f_dentry->d_inode, MAY_READ, NULL)) { - bprm->interp_flags |= BINPRM_FLAGS_ENFORCE_NONDUMP; - } - /* install the binary's fd. it is done at the latest possible point - * because once it is installed it will need to be sys_close()ed - * in case of error. - */ - fd_install (fd_binary, binary_file); - } - retval = search_binary_handler (bprm, regs); - if (retval < 0) - goto _error_close_file; - + goto _error; +#if 0 + if (files) { + steal_locks(files); + put_files_struct(files); + files = NULL; + } +#endif _ret: return retval; - -_error_close_file: - if (fd_binary > 0) { - sys_close (fd_binary); - fd_binary = -1; - bprm->file = NULL; - } _error: if (fd_binary > 0) - put_unused_fd (fd_binary); + sys_close(fd_binary); bprm->interp_flags = 0; +_unshare: + if (files) { + put_files_struct(current->files); + current->files = files; + } goto _ret; - } /* Command parsers */ @@ -302,7 +294,7 @@ static inline char * check_special_flags } /* * This registers a new binary format, it recognises the syntax - * ':name:type:offset:magic:mask:interpreter:' + * ':name:type:offset:magic:mask:interpreter:flags' * where the ':' is the IFS, that can be chosen with the first char */ static Node *create_entry(const char *buffer, size_t count) _