[PATCH] CRED: Fix check_unsafe_exec()

[PATCH] CRED: Fix check_unsafe_exec()

[David Howells]

check_unsafe_exec() relies on the usage counts of fs_struct and files_struct to
indicate the subscription count of cloned processes to these structures.  This
is not a viable method, however, as /proc can increment these counts when
merely accessing the data.

The effect of this is to occasionally prevent setuid executables from altering
their security details correctly.

To deal with this, subscription counters are added in addition to the usage
counters to fs_struct and files_struct.

This should hopefully fix:

	http://lkml.org/lkml/2009/2/25/491
	Date: Wed, 25 Feb 2009 18:11:54 -0500 (EST)
	From: Joe Malicki <jmalicki@metacarta.com>
	Subject: BUG: setuid sometimes doesn't.

	Very rarely, we experience a setuid program not properly getting
	the euid of its owner.  This happens with (at least) both Linux 2.6.24.7
	and Linux 2.6.28.4, on multiple machines of at least two configurations
	(Dell 860 and Dell 2950 - cpuinfo attached).
	...

Reported-by: Joe Malicki <jmalicki@metacarta.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/exec.c                 |    4 ++--
 fs/file.c                 |    1 +
 include/linux/fdtable.h   |    4 +++-
 include/linux/fs_struct.h |    7 ++++++-
 kernel/exit.c             |    2 ++
 kernel/fork.c             |    3 +++
 6 files changed, 17 insertions(+), 4 deletions(-)


diff --git a/fs/exec.c b/fs/exec.c
index 929b580..67d7a45 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1069,8 +1069,8 @@ void check_unsafe_exec(struct linux_binprm *bprm, struct files_struct *files)
 		n_sighand++;
 	}
 
-	if (atomic_read(&p->fs->count) > n_fs ||
-	    atomic_read(&p->files->count) > n_files ||
+	if (atomic_read(&p->fs->subscribers) > n_fs ||
+	    atomic_read(&p->files->subscribers) > n_files ||
 	    atomic_read(&p->sighand->count) > n_sighand)
 		bprm->unsafe |= LSM_UNSAFE_SHARE;
 
diff --git a/fs/file.c b/fs/file.c
index f313314..6a33a7a 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -303,6 +303,7 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 		goto out;
 
 	atomic_set(&newf->count, 1);
+	atomic_set(&newf->subscribers, 1);
 
 	spin_lock_init(&newf->file_lock);
 	newf->next_fd = 0;
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 09d6c5b..12e54bc 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -42,7 +42,9 @@ struct files_struct {
   /*
    * read mostly part
    */
-	atomic_t count;
+	atomic_t count;		/* number of processes accessing this set */
+	atomic_t subscribers;	/* number of cloned processes subscribed to
+				 * this set */
 	struct fdtable *fdt;
 	struct fdtable fdtab;
   /*
diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
index a97c053..47679c1 100644
--- a/include/linux/fs_struct.h
+++ b/include/linux/fs_struct.h
@@ -3,8 +3,13 @@
 
 #include <linux/path.h>
 
+/*
+ * General filesystem access parameter block.
+ */
 struct fs_struct {
-	atomic_t count;
+	atomic_t count;		/* number of processes accessing this block */
+	atomic_t subscribers;	/* number of cloned processes subscribed to
+				 * this block */
 	rwlock_t lock;
 	int umask;
 	struct path root, pwd;
diff --git a/kernel/exit.c b/kernel/exit.c
index efd30cc..57b63bb 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -561,6 +561,7 @@ void exit_files(struct task_struct *tsk)
 		task_lock(tsk);
 		tsk->files = NULL;
 		task_unlock(tsk);
+		atomic_dec(&files->subscribers);
 		put_files_struct(files);
 	}
 }
@@ -583,6 +584,7 @@ void exit_fs(struct task_struct *tsk)
 		task_lock(tsk);
 		tsk->fs = NULL;
 		task_unlock(tsk);
+		atomic_dec(&fs->subscribers);
 		put_fs_struct(fs);
 	}
 }
diff --git a/kernel/fork.c b/kernel/fork.c
index 4854c2c..9d1a2a7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -682,6 +682,7 @@ static struct fs_struct *__copy_fs_struct(struct fs_struct *old)
 	/* We don't need to lock fs - think why ;-) */
 	if (fs) {
 		atomic_set(&fs->count, 1);
+		atomic_set(&fs->subscribers, 1);
 		rwlock_init(&fs->lock);
 		fs->umask = old->umask;
 		read_lock(&old->lock);
@@ -705,6 +706,7 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk)
 {
 	if (clone_flags & CLONE_FS) {
 		atomic_inc(&current->fs->count);
+		atomic_inc(&current->fs->subscribers);
 		return 0;
 	}
 	tsk->fs = __copy_fs_struct(current->fs);
@@ -727,6 +729,7 @@ static int copy_files(unsigned long clone_flags, struct task_struct * tsk)
 
 	if (clone_flags & CLONE_FILES) {
 		atomic_inc(&oldf->count);
+		atomic_inc(&oldf->subscribers);
 		goto out;
 	}
 

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Hugh Dickins]

On Tue, 10 Mar 2009, David Howells wrote:

> check_unsafe_exec() relies on the usage counts of fs_struct and files_struct to
> indicate the subscription count of cloned processes to these structures.  This
> is not a viable method, however, as /proc can increment these counts when
> merely accessing the data.
> 
> The effect of this is to occasionally prevent setuid executables from altering
> their security details correctly.
> 
> To deal with this, subscription counters are added in addition to the usage
> counters to fs_struct and files_struct.
> 
> This should hopefully fix:
> 
> 	http://lkml.org/lkml/2009/2/25/491
> 	Date: Wed, 25 Feb 2009 18:11:54 -0500 (EST)
> 	From: Joe Malicki <jmalicki@metacarta.com>
> 	Subject: BUG: setuid sometimes doesn't.
> 
> 	Very rarely, we experience a setuid program not properly getting
> 	the euid of its owner.  This happens with (at least) both Linux 2.6.24.7
> 	and Linux 2.6.28.4, on multiple machines of at least two configurations
> 	(Dell 860 and Dell 2950 - cpuinfo attached).
> 	...
> 
> Reported-by: Joe Malicki <jmalicki@metacarta.com>
> Signed-off-by: David Howells <dhowells@redhat.com>

My current, certainly not to be trusted, belief is that this is
unnecessary overkill - as I've already suggested in private mail.

Surely we'd prefer to avoid the overhead of additional confusing
counts if they can be avoided?

We already have what I think is a satisfactory patch for the struct fs
part of it: /proc can easily manage root and pwd while holding the lock
instead of raising fs->count.

I don't understand why check_unsafe_exec() needs to check
current->files->count at all, since do_execve() has already
done an unshare_files() to get its own copy - and proceeds with
that one if the exec succeeds.

My belief is that the files->count check could/should have been
removed when that unshare_files() was put in.  Please explain why
I'm wrong on that - I can quite accept that I'm muddled about it,
but please do explain it to me.

Hugh

> ---
> 
>  fs/exec.c                 |    4 ++--
>  fs/file.c                 |    1 +
>  include/linux/fdtable.h   |    4 +++-
>  include/linux/fs_struct.h |    7 ++++++-
>  kernel/exit.c             |    2 ++
>  kernel/fork.c             |    3 +++
>  6 files changed, 17 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 929b580..67d7a45 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1069,8 +1069,8 @@ void check_unsafe_exec(struct linux_binprm *bprm, struct files_struct *files)
>  		n_sighand++;
>  	}
>  
> -	if (atomic_read(&p->fs->count) > n_fs ||
> -	    atomic_read(&p->files->count) > n_files ||
> +	if (atomic_read(&p->fs->subscribers) > n_fs ||
> +	    atomic_read(&p->files->subscribers) > n_files ||
>  	    atomic_read(&p->sighand->count) > n_sighand)
>  		bprm->unsafe |= LSM_UNSAFE_SHARE;
>  
> diff --git a/fs/file.c b/fs/file.c
> index f313314..6a33a7a 100644
> --- a/fs/file.c
> +++ b/fs/file.c
> @@ -303,6 +303,7 @@ struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
>  		goto out;
>  
>  	atomic_set(&newf->count, 1);
> +	atomic_set(&newf->subscribers, 1);
>  
>  	spin_lock_init(&newf->file_lock);
>  	newf->next_fd = 0;
> diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
> index 09d6c5b..12e54bc 100644
> --- a/include/linux/fdtable.h
> +++ b/include/linux/fdtable.h
> @@ -42,7 +42,9 @@ struct files_struct {
>    /*
>     * read mostly part
>     */
> -	atomic_t count;
> +	atomic_t count;		/* number of processes accessing this set */
> +	atomic_t subscribers;	/* number of cloned processes subscribed to
> +				 * this set */
>  	struct fdtable *fdt;
>  	struct fdtable fdtab;
>    /*
> diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
> index a97c053..47679c1 100644
> --- a/include/linux/fs_struct.h
> +++ b/include/linux/fs_struct.h
> @@ -3,8 +3,13 @@
>  
>  #include <linux/path.h>
>  
> +/*
> + * General filesystem access parameter block.
> + */
>  struct fs_struct {
> -	atomic_t count;
> +	atomic_t count;		/* number of processes accessing this block */
> +	atomic_t subscribers;	/* number of cloned processes subscribed to
> +				 * this block */
>  	rwlock_t lock;
>  	int umask;
>  	struct path root, pwd;
> diff --git a/kernel/exit.c b/kernel/exit.c
> index efd30cc..57b63bb 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -561,6 +561,7 @@ void exit_files(struct task_struct *tsk)
>  		task_lock(tsk);
>  		tsk->files = NULL;
>  		task_unlock(tsk);
> +		atomic_dec(&files->subscribers);
>  		put_files_struct(files);
>  	}
>  }
> @@ -583,6 +584,7 @@ void exit_fs(struct task_struct *tsk)
>  		task_lock(tsk);
>  		tsk->fs = NULL;
>  		task_unlock(tsk);
> +		atomic_dec(&fs->subscribers);
>  		put_fs_struct(fs);
>  	}
>  }
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 4854c2c..9d1a2a7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -682,6 +682,7 @@ static struct fs_struct *__copy_fs_struct(struct fs_struct *old)
>  	/* We don't need to lock fs - think why ;-) */
>  	if (fs) {
>  		atomic_set(&fs->count, 1);
> +		atomic_set(&fs->subscribers, 1);
>  		rwlock_init(&fs->lock);
>  		fs->umask = old->umask;
>  		read_lock(&old->lock);
> @@ -705,6 +706,7 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk)
>  {
>  	if (clone_flags & CLONE_FS) {
>  		atomic_inc(&current->fs->count);
> +		atomic_inc(&current->fs->subscribers);
>  		return 0;
>  	}
>  	tsk->fs = __copy_fs_struct(current->fs);
> @@ -727,6 +729,7 @@ static int copy_files(unsigned long clone_flags, struct task_struct * tsk)
>  
>  	if (clone_flags & CLONE_FILES) {
>  		atomic_inc(&oldf->count);
> +		atomic_inc(&oldf->subscribers);
>  		goto out;
>  	}
>  

Re: [PATCH] CRED: Fix check_unsafe_exec()

[David Howells]

Hugh Dickins <hugh@veritas.com> wrote:

> Surely we'd prefer to avoid the overhead of additional confusing
> counts if they can be avoided?

As long as they are properly commented, it shouldn't be too confusing.

> We already have what I think is a satisfactory patch for the struct fs
> part of it:

We do?

> /proc can easily manage root and pwd while holding the lock
> instead of raising fs->count.

I'm assume you mean by extending the time we hold task->alloc_lock until we've
completed the path_get().

> I don't understand why check_unsafe_exec() needs to check
> current->files->count at all, since do_execve() has already
> done an unshare_files() to get its own copy - and proceeds with
> that one if the exec succeeds.
> 
> My belief is that the files->count check could/should have been
> removed when that unshare_files() was put in.  Please explain why
> I'm wrong on that - I can quite accept that I'm muddled about it,
> but please do explain it to me.

It seems you're right about that.  I think someone else on the security list
probably needs to answer that.

David

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Hugh Dickins]

On Tue, 10 Mar 2009, David Howells wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > Surely we'd prefer to avoid the overhead of additional confusing
> > counts if they can be avoided?
> 
> As long as they are properly commented, it shouldn't be too confusing.

I'd rather have one count that doesn't need commenting
to distinguish from it another: I believe we all would.

> 
> > We already have what I think is a satisfactory patch for the struct fs
> > part of it:
> 
> We do?

We do.  See the original thread.  It's here at
http://lkml.org/lkml/2009/2/26/233
and appended below for convenience.  We do know that patch did not
fix Joe's problem, and we don't yet know whether addressing the
files->count issue will actually fix it, but I'm hopeful.

> 
> > /proc can easily manage root and pwd while holding the lock
> > instead of raising fs->count.
> 
> I'm assume you mean by extending the time we hold task->alloc_lock until we've
> completed the path_get().

Yep.

> 
> > I don't understand why check_unsafe_exec() needs to check
> > current->files->count at all, since do_execve() has already
> > done an unshare_files() to get its own copy - and proceeds with
> > that one if the exec succeeds.
> > 
> > My belief is that the files->count check could/should have been
> > removed when that unshare_files() was put in.  Please explain why
> > I'm wrong on that - I can quite accept that I'm muddled about it,
> > but please do explain it to me.
> 
> It seems you're right about that.  I think someone else on the security list
> probably needs to answer that.

--- 2.6.28/fs/proc/base.c	2008-12-24 23:26:37.000000000 +0000
+++ linux/fs/proc/base.c	2009-02-26 15:39:00.000000000 +0000
@@ -148,15 +148,22 @@ static unsigned int pid_entry_count_dirs
 	return count;
 }
 
-static struct fs_struct *get_fs_struct(struct task_struct *task)
+static int get_fs_path(struct task_struct *task, struct path *path, bool root)
 {
 	struct fs_struct *fs;
+	int result = -ENOENT;
+
 	task_lock(task);
 	fs = task->fs;
-	if(fs)
-		atomic_inc(&fs->count);
+	if (fs) {
+		read_lock(&fs->lock);
+		*path = root ? fs->root : fs->pwd;
+		path_get(path);
+		read_unlock(&fs->lock);
+		result = 0;
+	}
 	task_unlock(task);
-	return fs;
+	return result;
 }
 
 static int get_nr_threads(struct task_struct *tsk)
@@ -174,42 +181,24 @@ static int get_nr_threads(struct task_st
 static int proc_cwd_link(struct inode *inode, struct path *path)
 {
 	struct task_struct *task = get_proc_task(inode);
-	struct fs_struct *fs = NULL;
 	int result = -ENOENT;
 
 	if (task) {
-		fs = get_fs_struct(task);
+		result = get_fs_path(task, path, 0);
 		put_task_struct(task);
 	}
-	if (fs) {
-		read_lock(&fs->lock);
-		*path = fs->pwd;
-		path_get(&fs->pwd);
-		read_unlock(&fs->lock);
-		result = 0;
-		put_fs_struct(fs);
-	}
 	return result;
 }
 
 static int proc_root_link(struct inode *inode, struct path *path)
 {
 	struct task_struct *task = get_proc_task(inode);
-	struct fs_struct *fs = NULL;
 	int result = -ENOENT;
 
 	if (task) {
-		fs = get_fs_struct(task);
+		result = get_fs_path(task, path, 1);
 		put_task_struct(task);
 	}
-	if (fs) {
-		read_lock(&fs->lock);
-		*path = fs->root;
-		path_get(&fs->root);
-		read_unlock(&fs->lock);
-		result = 0;
-		put_fs_struct(fs);
-	}
 	return result;
 }
 
@@ -567,7 +556,6 @@ static int mounts_open_common(struct ino
 	struct task_struct *task = get_proc_task(inode);
 	struct nsproxy *nsp;
 	struct mnt_namespace *ns = NULL;
-	struct fs_struct *fs = NULL;
 	struct path root;
 	struct proc_mounts *p;
 	int ret = -EINVAL;
@@ -581,22 +569,16 @@ static int mounts_open_common(struct ino
 				get_mnt_ns(ns);
 		}
 		rcu_read_unlock();
-		if (ns)
-			fs = get_fs_struct(task);
+		if (ns && get_fs_path(task, &root, 1) == 0)
+			ret = 0;
 		put_task_struct(task);
 	}
 
 	if (!ns)
 		goto err;
-	if (!fs)
+	if (ret)
 		goto err_put_ns;
 
-	read_lock(&fs->lock);
-	root = fs->root;
-	path_get(&root);
-	read_unlock(&fs->lock);
-	put_fs_struct(fs);
-
 	ret = -ENOMEM;
 	p = kmalloc(sizeof(struct proc_mounts), GFP_KERNEL);
 	if (!p)

Re: [PATCH] CRED: Fix check_unsafe_exec()

[David Howells]

Hugh Dickins <hugh@veritas.com> wrote:

> We do.  See the original thread.  It's here at
> http://lkml.org/lkml/2009/2/26/233
> and appended below for convenience.  We do know that patch did not
> fix Joe's problem, and we don't yet know whether addressing the
> files->count issue will actually fix it, but I'm hopeful.

Looks reasonable.  One thing that should be added, though, is a comment in
struct fs_struct to give a warning about the consequences of incrementing the
usage count for anything other than CLONE_FS.

David
---
From: David Howells <dhowells@redhat.com>
Subject: [PATCH] Annotate struct fs_struct's usage count to indicate the restrictions upon it

Annotate struct fs_struct's usage count to indicate the restrictions upon it.
It may not be incremented, except by clone(CLONE_FS), as this affects the
check in check_unsafe_exec() in fs/exec.c.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/fs_struct.h |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)


diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
index a97c053..b12ede4 100644
--- a/include/linux/fs_struct.h
+++ b/include/linux/fs_struct.h
@@ -4,7 +4,11 @@
 #include <linux/path.h>
 
 struct fs_struct {
-	atomic_t count;
+	atomic_t count;	/* This usage count is used by check_unsafe_exec() for
+			 * security checking purposes - therefore it may not be
+			 * incremented, except by clone(CLONE_FS).
+			 */
+
 	rwlock_t lock;
 	int umask;
 	struct path root, pwd;

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Hugh Dickins]

On Thu, 12 Mar 2009, David Howells wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > We do.  See the original thread.  It's here at
> > http://lkml.org/lkml/2009/2/26/233
> > and appended below for convenience.  We do know that patch did not
> > fix Joe's problem, and we don't yet know whether addressing the
> > files->count issue will actually fix it, but I'm hopeful.
> 
> Looks reasonable.

Thanks for taking a look.

Yes, I'm inclined to go with that, and removing the files->count
check from exec.c.  Joe, did you manage to try your testing with
my original patch plus that files->count check removed from 2.6.28's
unsafe_exec()?

Though I've since thought a better answer would probably be to unshare
fs and sighand from the exec'ing task in the same way that files is
unshared at the start, then I hope we wouldn't need to suppress setuid
in the case when any of those had been shared.

But I believe that course would make a slight difference to the behaviour
of the respective CLONE flags versus exec: I'd guess a difference that
nobody cares about, but my guesses don't count for much here, and I
really don't want to cause any regression.

Chris, have you had a chance to look at any of this yet?

> One thing that should be added, though, is a comment in
> struct fs_struct to give a warning about the consequences of incrementing the
> usage count for anything other than CLONE_FS.

Yes, that's a very sensible addition, thanks - if we do go this way,
rather than unsharing.  I'll hold on to this as one of a set of three:
my original fs->count avoidance one, your comment on that below, and
removing the files->count check from exec.c.

Since Joe's bug has been around forever (if it is what we think it
is), I'm disinclined to rush the fix - something nice to add to
-stable, rather than needing to squeeze into 2.6.29.

Hugh

> 
> David
> ---
> From: David Howells <dhowells@redhat.com>
> Subject: [PATCH] Annotate struct fs_struct's usage count to indicate the restrictions upon it
> 
> Annotate struct fs_struct's usage count to indicate the restrictions upon it.
> It may not be incremented, except by clone(CLONE_FS), as this affects the
> check in check_unsafe_exec() in fs/exec.c.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  include/linux/fs_struct.h |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> 
> diff --git a/include/linux/fs_struct.h b/include/linux/fs_struct.h
> index a97c053..b12ede4 100644
> --- a/include/linux/fs_struct.h
> +++ b/include/linux/fs_struct.h
> @@ -4,7 +4,11 @@
>  #include <linux/path.h>
>  
>  struct fs_struct {
> -	atomic_t count;
> +	atomic_t count;	/* This usage count is used by check_unsafe_exec() for
> +			 * security checking purposes - therefore it may not be
> +			 * incremented, except by clone(CLONE_FS).
> +			 */
> +
>  	rwlock_t lock;
>  	int umask;
>  	struct path root, pwd;

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Joe Malicki]

----- "Hugh Dickins" <hugh@veritas.com> wrote:

> On Thu, 12 Mar 2009, David Howells wrote:
> > Hugh Dickins <hugh@veritas.com> wrote:
> > 
> > > We do.  See the original thread.  It's here at
> > > http://lkml.org/lkml/2009/2/26/233
> > > and appended below for convenience.  We do know that patch did
> not
> > > fix Joe's problem, and we don't yet know whether addressing the
> > > files->count issue will actually fix it, but I'm hopeful.
> > 
> > Looks reasonable.
> 
> Thanks for taking a look.
> 
> Yes, I'm inclined to go with that, and removing the files->count
> check from exec.c.  Joe, did you manage to try your testing with
> my original patch plus that files->count check removed from 2.6.28's
> unsafe_exec()?

Sorry for not responding earlier.

I still got one failure with this new patch.  I added some printks
to illuminate exactly why it's failing when it fails to setuid, but
of course, since adding the printks I haven't reproduced yet.
 
Very weird...

Thanks,
Joe

> Since Joe's bug has been around forever (if it is what we think it
> is), I'm disinclined to rush the fix - something nice to add to
> -stable, rather than needing to squeeze into 2.6.29.
> 
> Hugh
> 

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Joe Malicki]

----- "Joe Malicki" <jmalicki@metacarta.com> wrote:

> ----- "Hugh Dickins" <hugh@veritas.com> wrote:
> 
> > On Thu, 12 Mar 2009, David Howells wrote:
> > > Hugh Dickins <hugh@veritas.com> wrote:
> > >
> > > > We do.  See the original thread.  It's here at
> > > > http://lkml.org/lkml/2009/2/26/233
> > > > and appended below for convenience.  We do know that patch did
> > not
> > > > fix Joe's problem, and we don't yet know whether addressing the
> > > > files->count issue will actually fix it, but I'm hopeful.
> > >
> > > Looks reasonable.
> >
> > Thanks for taking a look.
> >
> > Yes, I'm inclined to go with that, and removing the files->count
> > check from exec.c.  Joe, did you manage to try your testing with
> > my original patch plus that files->count check removed from 2.6.28's
> > unsafe_exec()?
> 
> Sorry for not responding earlier.
> 
> I still got one failure with this new patch.  I added some printks
> to illuminate exactly why it's failing when it fails to setuid, but
> of course, since adding the printks I haven't reproduced yet.
>

My tests were accidentally run without removing the files->count check.

The printks confirmed the failure case was the files->count check,
and removing the files->check has worked thus far (though I can't be
sure until after a day or two has gone by given how infrequent it is).

Thanks!
Joe

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Hugh Dickins]

On Tue, 17 Mar 2009, Joe Malicki wrote:
> ----- "Joe Malicki" <jmalicki@metacarta.com> wrote:
> > ----- "Hugh Dickins" <hugh@veritas.com> wrote:
> > > Yes, I'm inclined to go with that, and removing the files->count
> > > check from exec.c.  Joe, did you manage to try your testing with
> > > my original patch plus that files->count check removed from 2.6.28's
> > > unsafe_exec()?
> > 
> > Sorry for not responding earlier.
> > 
> > I still got one failure with this new patch.  I added some printks
> > to illuminate exactly why it's failing when it fails to setuid, but
> > of course, since adding the printks I haven't reproduced yet.
> 
> My tests were accidentally run without removing the files->count check.

Oh good, thanks Joe, I was hoping that would be the case.

> The printks confirmed the failure case was the files->count check,
> and removing the files->check has worked thus far (though I can't be
> sure until after a day or two has gone by given how infrequent it is).

Okay, unless we hear bad news from you later, I'll post the set of three
patches (removing the files->count check, then my original fs->count one,
plus David's comment) in a day or two, adding a few more Ccs.

Hugh

Re: [PATCH] CRED: Fix check_unsafe_exec()

[Joe Malicki]

----- "Hugh Dickins" <hugh@veritas.com> wrote:

> 
> Okay, unless we hear bad news from you later, I'll post the set of
> three
> patches (removing the files->count check, then my original fs->count
> one,
> plus David's comment) in a day or two, adding a few more Ccs.
> 
> Hugh

No failures on several machines after several days... I'm pretty
satisfied that the patches fixed the problem.

Thank you so much for your work on this!
Joe Malicki