[PATCH] CRED: ptrace_attach() should use the target process's mutex

[PATCH] CRED: ptrace_attach() should use the target process's mutex

[David Howells]

ptrace_attach() should use the target process's mutex when attaching to it,
not the current (tracer) process's mutex.

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

 kernel/ptrace.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 893a099..2ee343a 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -174,7 +174,7 @@ int ptrace_attach(struct task_struct *task)
 	/* Protect exec's credential calculations against our interference;
 	 * SUID, SGID and LSM creds get determined differently under ptrace.
 	 */
-	retval = mutex_lock_interruptible(&current->cred_exec_mutex);
+	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
 	if (retval  < 0)
 		goto out;
 
@@ -218,7 +218,7 @@ repeat:
 bad:
 	write_unlock_irqrestore(&tasklist_lock, flags);
 	task_unlock(task);
-	mutex_unlock(&current->cred_exec_mutex);
+	mutex_unlock(&task->cred_exec_mutex);
 out:
 	return retval;
 }

Re: [PATCH] CRED: ptrace_attach() should use the target process's mutex

[James Morris]

On Wed, 1 Oct 2008, David Howells wrote:

> ptrace_attach() should use the target process's mutex when attaching to it,
> not the current (tracer) process's mutex.
> 
> Signed-off-by: David Howells <dhowells@redhat.com>

Applied to:
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next-creds-subsys


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH] CRED: ptrace_attach() should use the target process's mutex

[Tetsuo Handa]

Hello.

TOMOYO Linux is now trying to use CRED API, but some troubles were found.
Thus, I want the below patch for TOMOYO Linux.

Regards.
-----
Subject: Add hooks for notifying of start/finish of an execve operation.

This patch adds two hooks, security_start_execve() / security_finish_execve(),
for notifying an LSM module of an execve operation is about to start and
finish.

TOMOYO Linux checks read and/or write permissions at security_dentry_open()
if security_dentry_open() is called outside do_execve() (e.g. sys_open()).
But TOMOYO Linux wants to skip permission checks at security_dentry_open()
if security_dentry_open() is called inside do_execve() (i.e. open_exec()), for
TOMOYO Linux checks permissions at security_bprm_check() using
current->cred->security for program and bprm->cred->security for interpreter.

To implement this exception, I have to tell whether TOMOYO Linux should do
permission checks at security_dentry_open(). And I want a flag that tells
whether the current process is in an execve operation or not.

I tried to use

  static int tmy_dentry_open(struct file *f, const struct cred *cred)
  {
          ...
          /* Don't check read permission here if called from do_execve(). */
          if (mutex_is_locked(&current->cred_exec_mutex))
                  return 0;
          ...
  }

for judging whether the current process is in an execve operation or not.
But it turned out that this code won't work because current->cred_exec_mutex
can be locked by other processes. Thus, I'm trying to use

  struct execve_entry {
         struct list_head list;
         struct task_struct *task;
  };
  
  static LIST_HEAD(execve_list);
  static DEFINE_SPINLOCK(execve_list_lock);
  
  static int tmy_start_execve(void)
  {
          struct execve_entry *ee = kmalloc(sizeof(*ee), GFP_KERNEL);
          if (!ee)
                 return -ENOMEM;
          ee->task = current;
          spin_lock(&execve_list_lock);
          list_add(&ee->list, &execve_list);
          spin_unlock(&execve_list_lock);
          return 0;
  }
  
  static bool tmy_in_execve(void)
  {
          struct task_struct *task = current;
          struct execve_entry *p;
          bool found = false;
          spin_lock(&execve_list_lock);
          list_for_each_entry(p, &execve_list, list) {
                  if (p->task != task)
                          continue;
                  found = true;
                  break;
          }
          spin_unlock(&execve_list_lock);
          return found;
  }
  
  static void tmy_finish_execve(void)
  {
          struct task_struct *task = current;
          struct execve_entry *p;
          struct execve_entry *ee = NULL;
          spin_lock(&execve_list_lock);
          list_for_each_entry(p, &execve_list, list) {
                  if (p->task != task)
                          continue;
                  list_del(&p->list);
                  ee = p;
                  break;
          }
          spin_unlock(&execve_list_lock);
          if (ee)
                 kfree(ee);
  }

and replace mutex_is_locked(&current->cred_exec_mutex) with tmy_in_execve().
To call tmy_start_execve() and tmy_finish_execve(),
I want security_start_execve() / security_finish_execve().

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/compat.c              |    6 ++++++
 fs/exec.c                |    6 ++++++
 include/linux/security.h |   22 ++++++++++++++++++++++
 security/capability.c    |   11 +++++++++++
 security/security.c      |   10 ++++++++++
 5 files changed, 55 insertions(+)

--- linux-2.6.27-rc7-mm1.orig/fs/compat.c
+++ linux-2.6.27-rc7-mm1/fs/compat.c
@@ -1397,6 +1397,10 @@ int compat_do_execve(char * filename,
 	if (retval < 0)
 		goto out_free;
 
+	retval = security_start_execve();
+	if (retval < 0)
+		goto out_unlock;
+
 	retval = -ENOMEM;
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
@@ -1448,6 +1452,7 @@ int compat_do_execve(char * filename,
 		goto out;
 
 	/* execve succeeded */
+	security_finish_execve();
 	mutex_unlock(&current->cred_exec_mutex);
 	acct_update_integrals(current);
 	free_bprm(bprm);
@@ -1464,6 +1469,7 @@ out_file:
 	}
 
 out_unlock:
+	security_finish_execve();
 	mutex_unlock(&current->cred_exec_mutex);
 
 out_free:
--- linux-2.6.27-rc7-mm1.orig/fs/exec.c
+++ linux-2.6.27-rc7-mm1/fs/exec.c
@@ -1302,6 +1302,10 @@ int do_execve(char * filename,
 	if (retval < 0)
 		goto out_free;
 
+	retval = security_start_execve();
+	if (retval < 0)
+		goto out_unlock;
+
 	retval = -ENOMEM;
 	bprm->cred = prepare_exec_creds();
 	if (!bprm->cred)
@@ -1354,6 +1358,7 @@ int do_execve(char * filename,
 		goto out;
 
 	/* execve succeeded */
+	security_finish_execve();
 	mutex_unlock(&current->cred_exec_mutex);
 	acct_update_integrals(current);
 	free_bprm(bprm);
@@ -1372,6 +1377,7 @@ out_file:
 	}
 
 out_unlock:
+	security_finish_execve();
 	mutex_unlock(&current->cred_exec_mutex);
 
 out_free:
--- linux-2.6.27-rc7-mm1.orig/include/linux/security.h
+++ linux-2.6.27-rc7-mm1/include/linux/security.h
@@ -192,6 +192,15 @@ static inline void security_free_mnt_opt
  *	on the initial stack to the ELF interpreter to indicate whether libc
  *	should enable secure mode.
  *	@bprm contains the linux_binprm structure.
+ * @start_execve:
+ *	Notify current process of an execve operation is about to start.
+ *	This is called immediately after obtaining current->cred_exec_mutex
+ *	mutex.
+ *	Return 0 if operation was successful.
+ * @finish_execve:
+ *	Notify current process of an execve operation is about to finish.
+ *	This is called immediately before releasing current->cred_exec_mutex
+ *	mutex.
  *
  * Security hooks for filesystem operations.
  *
@@ -1352,6 +1361,8 @@ struct security_operations {
 	int (*settime) (struct timespec *ts, struct timezone *tz);
 	int (*vm_enough_memory) (struct mm_struct *mm, long pages);
 
+	int (*start_execve) (void);
+	void (*finish_execve) (void);
 	int (*bprm_set_creds) (struct linux_binprm *bprm);
 	int (*bprm_check_security) (struct linux_binprm *bprm);
 	int (*bprm_secureexec) (struct linux_binprm *bprm);
@@ -1632,6 +1643,8 @@ int security_syslog(int type);
 int security_settime(struct timespec *ts, struct timezone *tz);
 int security_vm_enough_memory(long pages);
 int security_vm_enough_memory_mm(struct mm_struct *mm, long pages);
+int security_start_execve(void);
+void security_finish_execve(void);
 int security_bprm_set_creds(struct linux_binprm *bprm);
 int security_bprm_check(struct linux_binprm *bprm);
 void security_bprm_committing_creds(struct linux_binprm *bprm);
@@ -1879,6 +1892,15 @@ static inline int security_vm_enough_mem
 	return cap_vm_enough_memory(mm, pages);
 }
 
+static inline int security_start_execve(void)
+{
+	return 0;
+}
+
+static inline void security_finish_execve(void)
+{
+}
+
 static inline int security_bprm_set_creds(struct linux_binprm *bprm)
 {
 	return cap_bprm_set_creds(bprm);
--- linux-2.6.27-rc7-mm1.orig/security/capability.c
+++ linux-2.6.27-rc7-mm1/security/capability.c
@@ -32,6 +32,15 @@ static int cap_quota_on(struct dentry *d
 	return 0;
 }
 
+static int cap_start_execve(void)
+{
+	return 0;
+}
+
+static void cap_finish_execve(void)
+{
+}
+
 static int cap_bprm_check_security (struct linux_binprm *bprm)
 {
 	return 0;
@@ -877,6 +886,8 @@ void security_fixup_ops(struct security_
 	set_to_cap_if_null(ops, syslog);
 	set_to_cap_if_null(ops, settime);
 	set_to_cap_if_null(ops, vm_enough_memory);
+	set_to_cap_if_null(ops, start_execve);
+	set_to_cap_if_null(ops, finish_execve);
 	set_to_cap_if_null(ops, bprm_set_creds);
 	set_to_cap_if_null(ops, bprm_committing_creds);
 	set_to_cap_if_null(ops, bprm_committed_creds);
--- linux-2.6.27-rc7-mm1.orig/security/security.c
+++ linux-2.6.27-rc7-mm1/security/security.c
@@ -199,6 +199,16 @@ int security_vm_enough_memory_mm(struct 
 	return security_ops->vm_enough_memory(mm, pages);
 }
 
+int security_start_execve(void)
+{
+	return security_ops->start_execve();
+}
+
+void security_finish_execve(void)
+{
+	security_ops->finish_execve();
+}
+
 int security_bprm_set_creds(struct linux_binprm *bprm)
 {
 	return security_ops->bprm_set_creds(bprm);

Re: [PATCH] CRED: ptrace_attach() should use the target process\'s mutex

[Tetsuo Handa]

Hello.

David Howells wrote:
> Tetsuo Handa wrote:
> 
> > Thus, I'm trying to use
> > 
> >   struct execve_entry {
> >          struct list_head list;
> >          struct task_struct *task;
> >   };
> > ...
> 
> It might be better to see about setting a flag in the task_struct.  All you
> need is a single bit, and since it's process-local it doesn't need any funny
> locking stuff.

Indeed. If I can use one bit in the task_struct, I don't need to add
security_start_execve() / security_finish_execve() hooks.

Now, I cancel the previous patch and propose this patch.

Thanks.
----------
Subject: Add in_execve flag into task_struct.

This patch allows LSM modules determine whether current process is in an execve
operation or not so that they can behave differently while an execve operation
is in progress.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/compat.c           |    4 ++++
 fs/exec.c             |    4 ++++
 include/linux/sched.h |    1 +
 3 files changed, 9 insertions(+)

--- linux-2.6.27-rc8-mm1.orig/fs/compat.c
+++ linux-2.6.27-rc8-mm1/fs/compat.c
@@ -1387,6 +1387,8 @@ int compat_do_execve(char * filename,
 	struct linux_binprm *bprm;
 	struct file *file;
 	int retval;
+	struct task_struct *task = current;
+	task->in_execve = 1;
 
 	retval = -ENOMEM;
 	bprm = kzalloc(sizeof(*bprm), GFP_KERNEL);
@@ -1443,6 +1445,7 @@ int compat_do_execve(char * filename,
 		security_bprm_free(bprm);
 		acct_update_integrals(current);
 		free_bprm(bprm);
+		task->in_execve = 0;
 		return retval;
 	}
 
@@ -1464,6 +1467,7 @@ out_kfree:
 	free_bprm(bprm);
 
 out_ret:
+	task->in_execve = 0;
 	return retval;
 }
 
--- linux-2.6.27-rc8-mm1.orig/fs/exec.c
+++ linux-2.6.27-rc8-mm1/fs/exec.c
@@ -1275,6 +1275,8 @@ int do_execve(char * filename,
 	struct file *file;
 	struct files_struct *displaced;
 	int retval;
+	struct task_struct *task = current;
+	task->in_execve = 1;
 
 	retval = unshare_files(&displaced);
 	if (retval)
@@ -1338,6 +1340,7 @@ int do_execve(char * filename,
 		free_bprm(bprm);
 		if (displaced)
 			put_files_struct(displaced);
+		task->in_execve = 0;
 		return retval;
 	}
 
@@ -1361,6 +1364,7 @@ out_files:
 	if (displaced)
 		reset_files_struct(displaced);
 out_ret:
+	task->in_execve = 0;
 	return retval;
 }
 
--- linux-2.6.27-rc8-mm1.orig/include/linux/sched.h
+++ linux-2.6.27-rc8-mm1/include/linux/sched.h
@@ -1088,6 +1088,7 @@ struct task_struct {
 	/* ??? */
 	unsigned int personality;
 	unsigned did_exec:1;
+	unsigned in_execve:1;
 	pid_t pid;
 	pid_t tgid;
 

Re: [PATCH] CRED: ptrace_attach() should use the target process\'s mutex

[David Howells]

Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:

> Subject: Add in_execve flag into task_struct.
> 
> This patch allows LSM modules determine whether current process is in an
> execve operation or not so that they can behave differently while an execve
> operation is in progress.

This doesn't apply on top of my patches.  How about the attached variant?

I've also:

 (1) Extended the patch description.  Please check that I've summed up TOMOYA's
     process correctly.

 (2) Used current directly rather than caching it in a variable.  I don't
     think it's worth doing that, given the amount you make use of it, but I
     can revert this change if you'd really prefer.

David
---
From: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>

CRED: Add in_execve flag into task_struct.

This patch allows LSM modules to determine whether current process is in an
execve operation or not so that they can behave differently while an execve
operation is in progress.

This allows TOMOYA to dispense with a readability check on a file to be
executed under the process's current credentials, and to do it instead under
the proposed credentials.

This is required with the new COW credentials because TOMOYA is no longer
allowed to mark the state temporarily in the security struct attached to the
task_struct.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/compat.c           |    3 +++
 fs/exec.c             |    3 +++
 include/linux/sched.h |    2 ++
 3 files changed, 8 insertions(+), 0 deletions(-)


diff --git a/fs/compat.c b/fs/compat.c
index 0c400ad..9e0dc12 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1355,6 +1355,7 @@ int compat_do_execve(char * filename,
 	struct file *file;
 	int retval;
 
+	current->in_execve = 1;
 	retval = -ENOMEM;
 	bprm = kzalloc(sizeof(*bprm), GFP_KERNEL);
 	if (!bprm)
@@ -1418,6 +1419,7 @@ int compat_do_execve(char * filename,
 	mutex_unlock(&current->cred_exec_mutex);
 	acct_update_integrals(current);
 	free_bprm(bprm);
+	current->in_execve = 0;
 	return retval;
 
 out:
@@ -1437,6 +1439,7 @@ out_free:
 	free_bprm(bprm);
 
 out_ret:
+	current->in_execve = 0;
 	return retval;
 }
 
diff --git a/fs/exec.c b/fs/exec.c
index a03a435..c1aab0c 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1296,6 +1296,7 @@ int do_execve(char * filename,
 	struct files_struct *displaced;
 	int retval;
 
+	current->in_execve = 1;
 	retval = unshare_files(&displaced);
 	if (retval)
 		goto out_ret;
@@ -1366,6 +1367,7 @@ int do_execve(char * filename,
 	free_bprm(bprm);
 	if (displaced)
 		put_files_struct(displaced);
+	current->in_execve = 0;
 	return retval;
 
 out:
@@ -1388,6 +1390,7 @@ out_files:
 	if (displaced)
 		reset_files_struct(displaced);
 out_ret:
+	current->in_execve = 0;
 	return retval;
 }
 
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 2ae6ef8..2648559 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1052,6 +1052,8 @@ struct task_struct {
 	/* ??? */
 	unsigned int personality;
 	unsigned did_exec:1;
+	unsigned in_execve:1;	/* Tell the LSMs that the process is doing an
+				 * execve */
 	pid_t pid;
 	pid_t tgid;
 

Re: [PATCH] CRED: ptrace_attach() should use the target process\'s mutex

[Tetsuo Handa]

Hello.

David Howells wrote:
> 
> Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote:
> 
> > Subject: Add in_execve flag into task_struct.
> > 
> > This patch allows LSM modules determine whether current process is in an
> > execve operation or not so that they can behave differently while an execve
> > operation is in progress.
> 
> This doesn't apply on top of my patches.  How about the attached variant?

Yes. Behavior will be the same.

> I've also:
> 
>  (1) Extended the patch description.  Please check that I've summed up TOMOYA's
>      process correctly.

Description is correct. But please apply "sed -e 's/TOMOYA/TOMOYO/g'".
TOMOYO Linux was introduced as TOMOYA Linux by mistake at OLS2008.
The origin of the name is at http://I-love.SAKURA.ne.jp/tomoyo/index.html.en :-)

>  (2) Used current directly rather than caching it in a variable.  I don't
>      think it's worth doing that, given the amount you make use of it, but I
>      can revert this change if you'd really prefer.

OK.

Please proceed. Thanks.