linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hugh Dickins <hugh@veritas.com>
To: Al Viro <viro@zeniv.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Joe Malicki <jmalicki@metacarta.com>,
	Michael Itz <mitz@metacarta.com>,
	Kenneth Baker <bakerk@metacarta.com>,
	Chris Wright <chrisw@sous-sol.org>,
	David Howells <dhowells@redhat.com>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>,
	Greg Kroah-Hartman <gregkh@suse.de>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [PATCH 2/4] fix setuid sometimes doesn't
Date: Sat, 28 Mar 2009 23:20:19 +0000 (GMT)	[thread overview]
Message-ID: <Pine.LNX.4.64.0903282319270.15432@blonde.anvils> (raw)
In-Reply-To: <Pine.LNX.4.64.0903282307050.14892@blonde.anvils>

Joe Malicki reports that setuid sometimes doesn't: very rarely,
a setuid root program does not get root euid; and, by the way,
they have a health check running lsof every few minutes.

Right, check_unsafe_exec() notes whether the files_struct is being
shared by more threads than will get killed by the exec, and if so
sets LSM_UNSAFE_SHARE to make bprm_set_creds() careful about euid.
But /proc/<pid>/fd and /proc/<pid>/fdinfo lookups make transient
use of get_files_struct(), which also raises that sharing count.

There's a rather simple fix for this: exec's check on files->count
has been redundant ever since 2.6.1 made it unshare_files() (except
while compat_do_execve() omitted to do so) - just remove that check.

[Note to -stable: this patch will not apply before 2.6.29: earlier
releases should just remove the files->count line from unsafe_exec().]

Reported-by: Joe Malicki <jmalicki@metacarta.com>
Narrowed-down-by: Michael Itz <mitz@metacarta.com>
Tested-by: Joe Malicki <jmalicki@metacarta.com>
Signed-off-by: Hugh Dickins <hugh@veritas.com>
Cc: stable@kernel.org
---

 fs/compat.c   |    2 +-
 fs/exec.c     |   10 +++-------
 fs/internal.h |    2 +-
 3 files changed, 5 insertions(+), 9 deletions(-)

--- 2.6.29/fs/compat.c	2009-03-23 23:12:14.000000000 +0000
+++ linux/fs/compat.c	2009-03-28 18:06:02.000000000 +0000
@@ -1412,7 +1412,7 @@ int compat_do_execve(char * filename,
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
 		goto out_unlock;
-	check_unsafe_exec(bprm, current->files);
+	check_unsafe_exec(bprm);
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
--- 2.6.29/fs/exec.c	2009-03-23 23:12:14.000000000 +0000
+++ linux/fs/exec.c	2009-03-28 18:06:02.000000000 +0000
@@ -1049,28 +1049,24 @@ EXPORT_SYMBOL(install_exec_creds);
  * - the caller must hold current->cred_exec_mutex to protect against
  *   PTRACE_ATTACH
  */
-void check_unsafe_exec(struct linux_binprm *bprm, struct files_struct *files)
+void check_unsafe_exec(struct linux_binprm *bprm)
 {
 	struct task_struct *p = current, *t;
 	unsigned long flags;
-	unsigned n_fs, n_files, n_sighand;
+	unsigned n_fs, n_sighand;
 
 	bprm->unsafe = tracehook_unsafe_exec(p);
 
 	n_fs = 1;
-	n_files = 1;
 	n_sighand = 1;
 	lock_task_sighand(p, &flags);
 	for (t = next_thread(p); t != p; t = next_thread(t)) {
 		if (t->fs == p->fs)
 			n_fs++;
-		if (t->files == files)
-			n_files++;
 		n_sighand++;
 	}
 
 	if (atomic_read(&p->fs->count) > n_fs ||
-	    atomic_read(&p->files->count) > n_files ||
 	    atomic_read(&p->sighand->count) > n_sighand)
 		bprm->unsafe |= LSM_UNSAFE_SHARE;
 
@@ -1289,7 +1285,7 @@ int do_execve(char * filename,
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
 		goto out_unlock;
-	check_unsafe_exec(bprm, displaced);
+	check_unsafe_exec(bprm);
 
 	file = open_exec(filename);
 	retval = PTR_ERR(file);
--- 2.6.29/fs/internal.h	2009-03-23 23:12:14.000000000 +0000
+++ linux/fs/internal.h	2009-03-28 18:06:02.000000000 +0000
@@ -43,7 +43,7 @@ extern void __init chrdev_init(void);
 /*
  * exec.c
  */
-extern void check_unsafe_exec(struct linux_binprm *, struct files_struct *);
+extern void check_unsafe_exec(struct linux_binprm *);
 
 /*
  * namespace.c

  reply	other threads:[~2009-03-28 23:21 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-03-28 23:16 [PATCH 1/4] compat_do_execve should unshare_files Hugh Dickins
2009-03-28 23:20 ` Hugh Dickins [this message]
2009-03-29  0:53   ` Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't) Oleg Nesterov
2009-03-29  4:10     ` Al Viro
2009-03-29  4:14       ` Al Viro
2009-03-29  4:52       ` Oleg Nesterov
2009-03-29  5:55         ` Al Viro
2009-03-29  6:01           ` Al Viro
2009-03-29 21:36             ` Oleg Nesterov
2009-03-29 22:20               ` Al Viro
2009-03-29 23:56                 ` Oleg Nesterov
2009-03-30  0:03                   ` Oleg Nesterov
2009-03-30  1:08                     ` Al Viro
2009-03-30  1:13                       ` Al Viro
2009-03-30  1:36                         ` Oleg Nesterov
2009-03-30  1:40                           ` Oleg Nesterov
2009-03-30 12:31                             ` Al Viro
2009-03-30 14:32                               ` Hugh Dickins
2009-03-31  6:16                                 ` Al Viro
2009-04-01  0:28                                   ` Hugh Dickins
2009-04-01  2:38                                     ` Al Viro
2009-04-01  3:03                                       ` Al Viro
2009-04-01 11:25                                         ` Hugh Dickins
2009-04-06 15:31                                         ` Oleg Nesterov
2009-04-19 16:30                                           ` Hugh Dickins
2009-04-21 16:10                                             ` Oleg Nesterov
2009-04-21 16:31                                               ` Linus Torvalds
2009-04-21 17:15                                                 ` Oleg Nesterov
2009-04-21 17:35                                                   ` Linus Torvalds
2009-04-21 19:39                                                     ` Hugh Dickins
2009-04-23 23:01                                                       ` [PATCH 1/2] do_execve() must not clear fs->in_exec if it was set by another thread Oleg Nesterov
2009-04-23 23:18                                                         ` Roland McGrath
2009-04-23 23:31                                                         ` Al Viro
2009-04-24 11:57                                                           ` [PATCH 3/2] check_unsafe_exec: rcu_read_unlock Hugh Dickins
2009-04-24 14:34                                                             ` Oleg Nesterov
2009-04-24  4:20                                                         ` [PATCH 1/2] do_execve() must not clear fs->in_exec if it was set by another thread Hugh Dickins
2009-04-23 23:02                                                       ` [PATCH 2/2] check_unsafe_exec: s/lock_task_sighand/rcu_read_lock/ Oleg Nesterov
2009-04-23 23:18                                                         ` Roland McGrath
2009-04-24  4:29                                                         ` Hugh Dickins
2009-04-01 11:18                                       ` Q: check_unsafe_exec() races (Was: [PATCH 2/4] fix setuid sometimes doesn't) Hugh Dickins
2009-04-06 15:51                                       ` Oleg Nesterov
2009-04-19 16:44                                         ` Hugh Dickins
2009-04-21 16:39                                           ` Oleg Nesterov
2009-03-30 23:45                               ` Serge E. Hallyn
2009-03-31  6:19                                 ` Al Viro
2009-03-28 23:21 ` [PATCH 3/4] fix setuid sometimes wouldn't Hugh Dickins
2009-03-29 11:19   ` Alexey Dobriyan
2009-03-29 21:48     ` Oleg Nesterov
2009-03-29 22:37       ` Al Viro
2009-03-28 23:23 ` [PATCH 4/4] Annotate struct fs_struct's usage count restriction Hugh Dickins

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Pine.LNX.4.64.0903282319270.15432@blonde.anvils \
    --to=hugh@veritas.com \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bakerk@metacarta.com \
    --cc=chrisw@sous-sol.org \
    --cc=dhowells@redhat.com \
    --cc=gregkh@suse.de \
    --cc=jmalicki@metacarta.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mitz@metacarta.com \
    --cc=oleg@redhat.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).