From: Chris Wright <chrisw@osdl.org>
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)
_