[PATCH 3/3] ptrace: do not use task_lock() for attach

[PATCH 3/3] ptrace: do not use task_lock() for attach

[Oleg Nesterov]

Remove the "Nasty, nasty" lock dance in ptrace_attach()/ptrace_traceme().
>From now task_lock() has nothing to do with ptrace at all.

With the recent changes nobody uses task_lock() to serialize with ptrace,
but in fact it was never needed and it was never used consistently.

However ptrace_attach() calls __ptrace_may_access() and needs task_lock()
to pin task->mm for get_dumpable(). But we can call __ptrace_may_access()
before we take tasklist_lock, ->cred_exec_mutex protects us against
do_execve() path which can change creds and MMF_DUMP* flags.

(ugly, but we can't use ptrace_may_access() because it hides the error
 code, so we have to take task_lock() and use __ptrace_may_access()).

NOTE: this change assumes that LSM hooks, security_ptrace_may_access() and
security_ptrace_traceme(), can be called without task_lock() held.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---

 kernel/ptrace.c |   59 ++++++++++++--------------------------------------------
 1 file changed, 13 insertions(+), 46 deletions(-)

--- PTRACE/kernel/ptrace.c~3_TASK_LOCK	2009-05-05 23:49:15.000000000 +0200
+++ PTRACE/kernel/ptrace.c	2009-05-06 00:16:17.000000000 +0200
@@ -177,7 +177,6 @@ bool ptrace_may_access(struct task_struc
 int ptrace_attach(struct task_struct *task)
 {
 	int retval;
-	unsigned long flags;
 
 	audit_ptrace(task);
 
@@ -193,34 +192,19 @@ int ptrace_attach(struct task_struct *ta
 	retval = mutex_lock_interruptible(&task->cred_exec_mutex);
 	if (retval < 0)
 		goto out;
-repeat:
-	/*
-	 * Nasty, nasty.
-	 *
-	 * We want to hold both the task-lock and the
-	 * tasklist_lock for writing at the same time.
-	 * But that's against the rules (tasklist_lock
-	 * is taken for reading by interrupts on other
-	 * cpu's that may have task_lock).
-	 */
-	task_lock(task);
-	if (!write_trylock_irqsave(&tasklist_lock, flags)) {
-		task_unlock(task);
-		do {
-			cpu_relax();
-		} while (!write_can_lock(&tasklist_lock));
-		goto repeat;
-	}
 
+	task_lock(task);
 	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+	task_unlock(task);
 	if (retval)
-		goto bad;
+		goto unlock_creds;
 
+	write_lock_irq(&tasklist_lock);
 	retval = -EPERM;
 	if (unlikely(task->exit_state))
-		goto bad;
+		goto unlock_tasklist;
 	if (task->ptrace)
-		goto bad;
+		goto unlock_tasklist;
 
 	task->ptrace = PT_PTRACED;
 	if (capable(CAP_SYS_PTRACE))
@@ -230,9 +214,9 @@ repeat:
 	send_sig_info(SIGSTOP, SEND_SIG_FORCED, task);
 
 	retval = 0;
-bad:
-	write_unlock_irqrestore(&tasklist_lock, flags);
-	task_unlock(task);
+unlock_tasklist:
+	write_unlock_irq(&tasklist_lock);
+unlock_creds:
 	mutex_unlock(&task->cred_exec_mutex);
 out:
 	return retval;
@@ -248,26 +232,10 @@ int ptrace_traceme(void)
 {
 	int ret = -EPERM;
 
-	/*
-	 * Are we already being traced?
-	 */
-repeat:
-	task_lock(current);
+	write_lock_irq(&tasklist_lock);
+	/* Are we already being traced? */
 	if (!current->ptrace) {
-		/*
-		 * See ptrace_attach() comments about the locking here.
-		 */
-		unsigned long flags;
-		if (!write_trylock_irqsave(&tasklist_lock, flags)) {
-			task_unlock(current);
-			do {
-				cpu_relax();
-			} while (!write_can_lock(&tasklist_lock));
-			goto repeat;
-		}
-
 		ret = security_ptrace_traceme(current->parent);
-
 		/*
 		 * Check PF_EXITING to ensure ->real_parent has not passed
 		 * exit_ptrace(). Otherwise we don't report the error but
@@ -277,10 +245,9 @@ repeat:
 			current->ptrace = PT_PTRACED;
 			__ptrace_link(current, current->real_parent);
 		}
-
-		write_unlock_irqrestore(&tasklist_lock, flags);
 	}
-	task_unlock(current);
+	write_unlock_irq(&tasklist_lock);
+
 	return ret;
 }
 

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

[RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> +	task_lock(task);
>  	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
> +	task_unlock(task);
>  	if (retval)
> -		goto bad;
> +		goto unlock_creds;

Hm, that's a bit ugly - why dont you reuse ptrace_may_access(), 
which does much of this already?

That way we could save a little bit of code size.

Something like the patch below allows the reuse of the locked 
version of __ptrace_may_access and pushes the int->bool conversion 
into an inline.

Note that this will also be a micro-optimization: the compiler will 
be able to eliminate the inlined negation in the call sites (most of 
which use the value directly) - and thus we save the negation.

( Untested - if you test it then it has my signoff. Patch also does 
  a small cleanup in _ptrace_may_access() )

Hm?

	Ingo

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 59e133d..2cc01ec 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -99,8 +99,13 @@ extern void exit_ptrace(struct task_struct *tracer);
 #define PTRACE_MODE_ATTACH 2
 /* Returns 0 on success, -errno on denial. */
 extern int __ptrace_may_access(struct task_struct *task, unsigned int mode);
-/* Returns true on success, false on denial. */
-extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
+/* Also takes the task lock: */
+extern int _ptrace_may_access(struct task_struct *task, unsigned int mode);
+/* True on success, false on denial: */
+static inline bool ptrace_may_access(struct task_struct *task, unsigned int mode)
+{
+	return !_ptrace_may_access(task, mode);
+}
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index e950805..f7efcc9 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -155,13 +155,15 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	return security_ptrace_may_access(task, mode);
 }
 
-bool ptrace_may_access(struct task_struct *task, unsigned int mode)
+int _ptrace_may_access(struct task_struct *task, unsigned int mode)
 {
 	int err;
+
 	task_lock(task);
 	err = __ptrace_may_access(task, mode);
 	task_unlock(task);
-	return !err;
+
+	return err;
 }
 
 int ptrace_attach(struct task_struct *task)

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Roland McGrath]

> Something like the patch below allows the reuse of the locked 
> version of __ptrace_may_access and pushes the int->bool conversion 
> into an inline.

I think it would be cleaner and safe/simple enough to invert the public
ptrace_may_access() to just return the int and invert the ! on all the
callers (all one in fs/proc/task_mmu.c and all four in fs/proc/base.c).


Thanks,
Roland

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Christoph Hellwig]

On Wed, May 06, 2009 at 01:32:24PM -0700, Roland McGrath wrote:
> > Something like the patch below allows the reuse of the locked 
> > version of __ptrace_may_access and pushes the int->bool conversion 
> > into an inline.
> 
> I think it would be cleaner and safe/simple enough to invert the public
> ptrace_may_access() to just return the int and invert the ! on all the
> callers (all one in fs/proc/task_mmu.c and all four in fs/proc/base.c).

Yeah.  And at the same time we might move it out of ptrace.c and give it
a more descriptive name given that most users aren't related to ptrace
in any way.  may_inspect_task maybe?  I'm good at naming things..

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Roland McGrath]

> Yeah.  And at the same time we might move it out of ptrace.c and give it
> a more descriptive name given that most users aren't related to ptrace
> in any way.  may_inspect_task maybe?  I'm good at naming things..

Yes, I would like to have it outside ptrace.c (I'd forgotten about that).
fs/proc/base.c is where most of the users really are, but CONFIG_PROC_FS=n
makes putting it there problematical.

The name probably should stay the same unless/until we rename the LSM hooks
to match.  TBH, two separate hooks would make more sense to me than the
PTRACE_MODE_* argument.  For the hook replacing PTRACE_MODE_READ, the name
may_inspect_task is apropos (not that I am attached to any given name).


Thanks,
Roland

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Christoph Hellwig <hch@infradead.org> wrote:

> On Wed, May 06, 2009 at 01:32:24PM -0700, Roland McGrath wrote:
> > > Something like the patch below allows the reuse of the locked 
> > > version of __ptrace_may_access and pushes the int->bool conversion 
> > > into an inline.
> > 
> > I think it would be cleaner and safe/simple enough to invert the public
> > ptrace_may_access() to just return the int and invert the ! on all the
> > callers (all one in fs/proc/task_mmu.c and all four in fs/proc/base.c).
> 
> Yeah.  And at the same time we might move it out of ptrace.c and 
> give it a more descriptive name given that most users aren't 
> related to ptrace in any way.  may_inspect_task maybe? [...]

That name (due to the 'may') signals/suggests a bool property as 
well, so if it has a retval==0 convention it's either incorrectly 
named, or the return convention need to stay a bool.

> [...]  I'm good at naming things..

not in this case ;-)

	Ingo

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> > Something like the patch below allows the reuse of the locked 
> > version of __ptrace_may_access and pushes the int->bool 
> > conversion into an inline.
> 
> I think it would be cleaner and safe/simple enough to invert the 
> public ptrace_may_access() to just return the int and invert the ! 
> on all the callers (all one in fs/proc/task_mmu.c and all four in 
> fs/proc/base.c).

hm, i considered that briefly and rejected the idea because 
something that says 'may' in its name is generally assumed to be a 
logic function-ish thing.

I.e. in such case: ptrace_may_access() or task_is_traced() people 
sub-consciously assume that 0 means "no", non-0 means "yes". So in 
that sense i liked the bool wrapper and preserved that in the 
inline.

We do have the retval==0-means-success convention in a number of 
APIs, but APIs that include a verb in their names assert some sort 
of property all have bool behavior.

(The underscore itself signals some special property - so there the 
deviation from the usual conventions isnt a big problem.)

This might sound like a nuance, but it really matters in the grand 
scheme of things. So IMHO inverting the logic is a step backwards - 
beyond the needless churn as well that it causes in various 
subsystems.

	Ingo

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Oleg Nesterov]

On 05/06, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > +	task_lock(task);
> >  	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
> > +	task_unlock(task);
> >  	if (retval)
> > -		goto bad;
> > +		goto unlock_creds;
>
> Hm, that's a bit ugly - why dont you reuse ptrace_may_access(),
> which does much of this already?

Indeed, even the changelog mentions this.

I was going to cleanup this later. Because I think that
__ptrace_may_access() should die. It has only one caller, mm_for_maps().
I will re-check, but it looks a bit strange. More precisely, I just
can't understand it. Why we can't just do

	struct mm_struct *mm_for_maps(struct task_struct *task)
	{
		struct mm_struct *mm = get_task_mm(task);

		if (mm && mm != current->mm && !ptrace_may_access()) {
			mmput(mm);
			mm = NULL;
		}

		return mm;
	}

? We do not care if this task exits and clears ->mm right before
or after ptrace_may_access(), and this is possible eith the current
code too once it drops tasklist.

Oleg.

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Roland McGrath]

> I was going to cleanup this later. Because I think that
> __ptrace_may_access() should die. It has only one caller, mm_for_maps().

CC'ing Al Viro, who wrote mm_for_maps() (and no one has touched it since,
see commit 831830b).

> I will re-check, but it looks a bit strange. More precisely, I just
> can't understand it. Why we can't just do
> 
> 	struct mm_struct *mm_for_maps(struct task_struct *task)
> 	{
> 		struct mm_struct *mm = get_task_mm(task);
> 
> 		if (mm && mm != current->mm && !ptrace_may_access()) {
> 			mmput(mm);
> 			mm = NULL;
> 		}
> 
> 		return mm;
> 	}

That seems fine to me.  I suspect the old code just predated the PF_KTHREAD
check in get_task_mm() and excluding the borrowed-mm window races was the
only reason for using task_lock() that way.

> ? We do not care if this task exits and clears ->mm right before
> or after ptrace_may_access(), and this is possible eith the current
> code too once it drops tasklist.

I agree.


Thanks,
Roland

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Oleg Nesterov]

On 05/06, Roland McGrath wrote:
>
> > I was going to cleanup this later. Because I think that
> > __ptrace_may_access() should die. It has only one caller, mm_for_maps().
>
> CC'ing Al Viro, who wrote mm_for_maps() (and no one has touched it since,
> see commit 831830b).
>
> > I will re-check, but it looks a bit strange. More precisely, I just
> > can't understand it. Why we can't just do
> >
> > 	struct mm_struct *mm_for_maps(struct task_struct *task)
> > 	{
> > 		struct mm_struct *mm = get_task_mm(task);
> >
> > 		if (mm && mm != current->mm && !ptrace_may_access()) {
> > 			mmput(mm);
> > 			mm = NULL;
> > 		}
> >
> > 		return mm;
> > 	}
>
> That seems fine to me.  I suspect the old code just predated the PF_KTHREAD
> check in get_task_mm() and excluding the borrowed-mm window races was the
> only reason for using task_lock() that way.
>
> > ? We do not care if this task exits and clears ->mm right before
> > or after ptrace_may_access(), and this is possible eith the current
> > code too once it drops tasklist.
>
> I agree.

Great. Will try to make the patches soon.

And I forgot to mention, there is another reason to kill __ptrace_may_access.
Because we can "narrow" the critical section protected by task_lock(). Not
for performance of course, just for clarity:

/* the callers of ptrace_may_access should be fixed */

	int ptrace_may_access(struct task_struct *task, unsigned int mode)
	{
		const struct cred *cred = current_cred(), *tcred;
		int ret = 0;

		/* May we inspect the given task?
		 * This check is used both for attaching with ptrace
		 * and for allowing access to sensitive information in /proc.
		 *
		 * ptrace_attach denies several cases that /proc allows
		 * because setting up the necessary parent/child relationship
		 * or halting the specified task is impossible.
		 */
		/* Don't let security modules deny introspection */
		if (task == current)
			return ret;

		rcu_read_lock();
		tcred = __task_cred(task);
		if ((cred->uid != tcred->euid ||
		     cred->uid != tcred->suid ||
		     cred->uid != tcred->uid  ||
		     cred->gid != tcred->egid ||
		     cred->gid != tcred->sgid ||
		     cred->gid != tcred->gid) &&
		    !capable(CAP_SYS_PTRACE))
			ret = -EPERM;
		rcu_read_unlock();
		if (ret)
			return ret;
/* kill rmb ? */

		task_lock(task);
		if (!task->mm || !get_dumpable(task->mm)) {
			if (!capable(CAP_SYS_PTRACE))
				ret = -EPERM;
		task_unclock(task);
		if (ret)
			return ret;

		return security_ptrace_may_access(task, mode);
	}

Btw, "[PATCH 3/3]" notes that security_ptrace_may_access() is called without
task_lock(), this note "leaked" from this change in future ;)

But firsty I'll try to grep/recheck this all.

Oleg.

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> /* the callers of ptrace_may_access should be fixed */
> 
> 	int ptrace_may_access(struct task_struct *task, unsigned int mode)

Sigh, NAK, for the reasons explained in the previous mails.

	Ingo

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Oleg Nesterov]

On 05/07, Ingo Molnar wrote:
>
> * Oleg Nesterov <oleg@redhat.com> wrote:
>
> > /* the callers of ptrace_may_access should be fixed */
> >
> > 	int ptrace_may_access(struct task_struct *task, unsigned int mode)
>
> Sigh, NAK, for the reasons explained in the previous mails.

Agreed, but what about security_operations->ptrace_may_access ?

It has the same (bad) name, but returns the error code or 0 on
success.

Oleg.

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/07, Ingo Molnar wrote:
> >
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > > /* the callers of ptrace_may_access should be fixed */
> > >
> > > 	int ptrace_may_access(struct task_struct *task, unsigned int mode)
> >
> > Sigh, NAK, for the reasons explained in the previous mails.
> 
> Agreed, but what about security_operations->ptrace_may_access ?
> 
> It has the same (bad) name, but returns the error code or 0 on 
> success.

Bad code should generally be fixed, or in exceptional circumstances 
it can tolerated if it's pre-existing bad code, but it should never 
be propagated. It has not spread _that_ widely yet, and is isolated 
to the security subsystem:

 include/linux/security.h
 security/capability.c
 security/commoncap.c
 security/root_plug.c
 security/security.c
 security/selinux/hooks.c
 security/smack/smack_lsm.c

	Ingo

[patch] security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > On 05/07, Ingo Molnar wrote:
> > >
> > > * Oleg Nesterov <oleg@redhat.com> wrote:
> > >
> > > > /* the callers of ptrace_may_access should be fixed */
> > > >
> > > > 	int ptrace_may_access(struct task_struct *task, unsigned int mode)
> > >
> > > Sigh, NAK, for the reasons explained in the previous mails.
> > 
> > Agreed, but what about security_operations->ptrace_may_access ?
> > 
> > It has the same (bad) name, but returns the error code or 0 on 
> > success.
> 
> Bad code should generally be fixed, or in exceptional circumstances 
> it can tolerated if it's pre-existing bad code, but it should never 
> be propagated. It has not spread _that_ widely yet, and is isolated 
> to the security subsystem:
> 
>  include/linux/security.h
>  security/capability.c
>  security/commoncap.c
>  security/root_plug.c
>  security/security.c
>  security/selinux/hooks.c
>  security/smack/smack_lsm.c

btw. the most logical way to fix this would be to rename it from 
ptrace_may_access to ptrace_access_check. Takes 30 seconds to do - 
find the (completely untested) patch for that below.

	Ingo

------------------->
Subject: security: rename ptrace_may_access => ptrace_access_check
From: Ingo Molnar <mingo@elte.hu>

The ->ptrace_may_access() methods are named confusingly - the real 
ptrace_may_access() returns a bool, while these security checks have 
a retval convention.

Rename it to ptrace_access_check, to reduce the confusion factor.

[ Impact: cleanup, no code changed ]

Signed-off-by-if-you-test-it: Ingo Molnar <mingo@elte.hu>
---
 include/linux/security.h   |   14 +++++++-------
 security/capability.c      |    2 +-
 security/commoncap.c       |    4 ++--
 security/root_plug.c       |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    6 +++---
 security/smack/smack_lsm.c |    8 ++++----
 7 files changed, 20 insertions(+), 20 deletions(-)

Index: tip/include/linux/security.h
===================================================================
--- tip.orig/include/linux/security.h
+++ tip/include/linux/security.h
@@ -52,7 +52,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1798,10 +1798,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/capability.c
===================================================================
--- tip.orig/security/capability.c
+++ tip/security/capability.c
@@ -867,7 +867,7 @@ struct security_operations default_secur
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
Index: tip/security/commoncap.c
===================================================================
--- tip.orig/security/commoncap.c
+++ tip/security/commoncap.c
@@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
Index: tip/security/root_plug.c
===================================================================
--- tip.orig/security/root_plug.c
+++ tip/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
Index: tip/security/security.c
===================================================================
--- tip.orig/security/security.c
+++ tip/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_op
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/selinux/hooks.c
===================================================================
--- tip.orig/security/selinux/hooks.c
+++ tip/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
Index: tip/security/smack/smack_lsm.c
===================================================================
--- tip.orig/security/smack/smack_lsm.c
+++ tip/security/smack/smack_lsm.c
@@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,

Re: [patch] security: rename ptrace_may_access => ptrace_access_check

[Oleg Nesterov]

On 05/07, Ingo Molnar wrote:
>
> * Ingo Molnar <mingo@elte.hu> wrote:
> Subject: security: rename ptrace_may_access => ptrace_access_check
> From: Ingo Molnar <mingo@elte.hu>
>
> The ->ptrace_may_access() methods are named confusingly - the real 
> ptrace_may_access() returns a bool, while these security checks have 
> a retval convention.
> 
> Rename it to ptrace_access_check, to reduce the confusion factor.

Great. Now we can rename (and fix the callers of) ptrace.c:ptrace_may_access()
accordingly.

But,

> -static inline int security_ptrace_may_access(struct task_struct *child,
> +static inline int security_ptrace_access_check(struct task_struct *child,
>  					     unsigned int mode)

You seem to forgot to update the callers of this helper.

Oleg.

Re: [patch] security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

* Oleg Nesterov <oleg@redhat.com> wrote:

> On 05/07, Ingo Molnar wrote:
> >
> > * Ingo Molnar <mingo@elte.hu> wrote:
> > Subject: security: rename ptrace_may_access => ptrace_access_check
> > From: Ingo Molnar <mingo@elte.hu>
> >
> > The ->ptrace_may_access() methods are named confusingly - the real 
> > ptrace_may_access() returns a bool, while these security checks have 
> > a retval convention.
> > 
> > Rename it to ptrace_access_check, to reduce the confusion factor.
> 
> Great. Now we can rename (and fix the callers of) ptrace.c:ptrace_may_access()
> accordingly.
> 
> But,
> 
> > -static inline int security_ptrace_may_access(struct task_struct *child,
> > +static inline int security_ptrace_access_check(struct task_struct *child,
> >  					     unsigned int mode)
> 
> You seem to forgot to update the callers of this helper.

Did i mention that it's completely untested :) Yeah, i'd suggest to 
push this naming down the whole ptrace_may_access landscape, and 
eliminate the bool. In two separate patches: first the rename, then 
the bool-elimination (which is more dangerous).

	Ingo

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Chris Wright]

* Ingo Molnar (mingo@elte.hu) wrote:
> * Oleg Nesterov <oleg@redhat.com> wrote:
> > Agreed, but what about security_operations->ptrace_may_access ?
> > It has the same (bad) name, but returns the error code or 0 on 
> > success.
> 
> Bad code should generally be fixed, or in exceptional circumstances 
> it can tolerated if it's pre-existing bad code, but it should never 
> be propagated. It has not spread _that_ widely yet, and is isolated 
> to the security subsystem:

And the security hooks tend to all follow the 0 success -ve ERR on error.

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Chris Wright <chrisw@sous-sol.org> wrote:

> * Ingo Molnar (mingo@elte.hu) wrote:
> > * Oleg Nesterov <oleg@redhat.com> wrote:
> > > Agreed, but what about security_operations->ptrace_may_access ?
> > > It has the same (bad) name, but returns the error code or 0 on 
> > > success.
> > 
> > Bad code should generally be fixed, or in exceptional circumstances 
> > it can tolerated if it's pre-existing bad code, but it should never 
> > be propagated. It has not spread _that_ widely yet, and is isolated 
> > to the security subsystem:
> 
> And the security hooks tend to all follow the 0 success -ve ERR on error.

I just sent a patch (see below) that renames them to 
ptrace_access_check().

They have no active connection to the core kernel 
ptrace_may_access() check in any case: the methods seem to be 
home-brewn, security-module specific checks for how wide ptrace is 
allowed to go.

The design around that code does not seem to be very consistent.

One solution would be for the default "plain Linux" security module 
to have a stock ->ptrace_access_check() that does the current 
ptrace_may_access() check, and then procfs could be updated to use 
that callback - instead of calling into the ptrace core code 
directly.

This would mean that in the end the 'may' usage is eliminated 
altogether, and we have a clean ptrace_access_check() facility with 
a retval convention.

	Ingo

-------------->
Subject: security: rename ptrace_may_access => ptrace_access_check
From: Ingo Molnar <mingo@elte.hu>

The ->ptrace_may_access methods are named confusingly - the real
ptrace_may_access() returns a bool, while these security checks have
a retval convention.

Rename it to ptrace_access_check, to reduce the confusion factor.

[ Impact: cleanup, no code changed ]

Signed-off-by-if-you-test-it: Ingo Molnar <mingo@elte.hu>
---
 include/linux/security.h   |   14 +++++++-------
 security/capability.c      |    2 +-
 security/commoncap.c       |    4 ++--
 security/root_plug.c       |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    6 +++---
 security/smack/smack_lsm.c |    8 ++++----
 7 files changed, 20 insertions(+), 20 deletions(-)

Index: tip/include/linux/security.h
===================================================================
--- tip.orig/include/linux/security.h
+++ tip/include/linux/security.h
@@ -52,7 +52,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1798,10 +1798,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/capability.c
===================================================================
--- tip.orig/security/capability.c
+++ tip/security/capability.c
@@ -867,7 +867,7 @@ struct security_operations default_secur
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
Index: tip/security/commoncap.c
===================================================================
--- tip.orig/security/commoncap.c
+++ tip/security/commoncap.c
@@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
Index: tip/security/root_plug.c
===================================================================
--- tip.orig/security/root_plug.c
+++ tip/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
Index: tip/security/security.c
===================================================================
--- tip.orig/security/security.c
+++ tip/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_op
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
Index: tip/security/selinux/hooks.c
===================================================================
--- tip.orig/security/selinux/hooks.c
+++ tip/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
Index: tip/security/smack/smack_lsm.c
===================================================================
--- tip.orig/security/smack/smack_lsm.c
+++ tip/security/smack/smack_lsm.c
@@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Chris Wright]

* Ingo Molnar (mingo@elte.hu) wrote:
> 
> * Chris Wright <chrisw@sous-sol.org> wrote:
> 
> > * Ingo Molnar (mingo@elte.hu) wrote:
> > > * Oleg Nesterov <oleg@redhat.com> wrote:
> > > > Agreed, but what about security_operations->ptrace_may_access ?
> > > > It has the same (bad) name, but returns the error code or 0 on 
> > > > success.
> > > 
> > > Bad code should generally be fixed, or in exceptional circumstances 
> > > it can tolerated if it's pre-existing bad code, but it should never 
> > > be propagated. It has not spread _that_ widely yet, and is isolated 
> > > to the security subsystem:
> > 
> > And the security hooks tend to all follow the 0 success -ve ERR on error.
> 
> I just sent a patch (see below) that renames them to 
> ptrace_access_check().
> 
> They have no active connection to the core kernel 
> ptrace_may_access() check in any case:

Not sure what you mean:

ptrace_may_access
 __ptrace_may_access
  security_ptrace_may_access

Looks like your patch won't compile.

(no subject)

[James Morris]

On Thu, 7 May 2009, Chris Wright wrote:

> * Ingo Molnar (mingo@elte.hu) wrote:

[Added LSM list to the CC; please do so whenever making changes in this 
area...]

> > They have no active connection to the core kernel 
> > ptrace_may_access() check in any case:
> 
> Not sure what you mean:
> 
> ptrace_may_access
>  __ptrace_may_access
>   security_ptrace_may_access
> 
> Looks like your patch won't compile.
> 

Below is an updated version which fixes the bug, against 
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

Boot tested with SELinux.

commit c4c79671177dc3e8387c337f75f3c664cdf08838
Author: Ingo Molnar <mingo@elte.hu>
Date:   Thu May 7 19:26:19 2009 +1000

    security: rename ptrace_may_access => ptrace_access_check
    
    The ->ptrace_may_access() methods are named confusingly - the real
    ptrace_may_access() returns a bool, while these security checks have
    a retval convention.
    
    Rename it to ptrace_access_check, to reduce the confusion factor.
    
    [ Impact: cleanup, no code changed ]
    
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: James Morris <jmorris@namei.org>

diff --git a/include/linux/security.h b/include/linux/security.h
index 54ed157..0147def 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -51,7 +51,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1208,7 +1208,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1223,7 +1223,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1335,7 +1335,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1616,7 +1616,7 @@ extern int security_module_enable(struct security_operations *ops);
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1797,10 +1797,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index c9cf48b..284d0ac 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -160,7 +160,7 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
 	if (!dumpable && !capable(CAP_SYS_PTRACE))
 		return -EPERM;
 
-	return security_ptrace_may_access(task, mode);
+	return security_ptrace_access_check(task, mode);
 }
 
 bool ptrace_may_access(struct task_struct *task, unsigned int mode)
diff --git a/security/capability.c b/security/capability.c
index 21b6cea..f218dd3 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -863,7 +863,7 @@ struct security_operations default_security_ops = {
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
diff --git a/security/commoncap.c b/security/commoncap.c
index 97ac1f1..e57611a 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -101,7 +101,7 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -109,7 +109,7 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
diff --git a/security/root_plug.c b/security/root_plug.c
index 40fb4f1..e8d5861 100644
--- a/security/root_plug.c
+++ b/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security (struct linux_binprm *bprm)
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
diff --git a/security/security.c b/security/security.c
index 206e538..a3e6918 100644
--- a/security/security.c
+++ b/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_operations *ops)
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 39046dd..e30c4bb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct file *file)
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5310,7 +5310,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index f557767..79949f9 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -91,7 +91,7 @@ struct inode_smack *new_inode_smack(char *smack)
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -99,13 +99,13 @@ struct inode_smack *new_inode_smack(char *smack)
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 	struct smk_audit_info ad;
 	char *sp, *tsp;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -3031,7 +3031,7 @@ static void smack_release_secctx(char *secdata, u32 seclen)
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,

Re: your mail

[Ingo Molnar]

* James Morris <jmorris@namei.org> wrote:

> On Thu, 7 May 2009, Chris Wright wrote:
> 
> > * Ingo Molnar (mingo@elte.hu) wrote:
> 
> [Added LSM list to the CC; please do so whenever making changes in this 
> area...]
> 
> > > They have no active connection to the core kernel 
> > > ptrace_may_access() check in any case:
> > 
> > Not sure what you mean:
> > 
> > ptrace_may_access
> >  __ptrace_may_access
> >   security_ptrace_may_access
> > 
> > Looks like your patch won't compile.
> > 
> 
> Below is an updated version which fixes the bug, against 
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
> 
> Boot tested with SELinux.

thanks! Below are the two patches i wrote and tested.

	Ingo

----- Forwarded message from Ingo Molnar <mingo@elte.hu> -----

Date: Thu, 7 May 2009 11:49:47 +0200
From: Ingo Molnar <mingo@elte.hu>
To: Chris Wright <chrisw@sous-sol.org>
Subject: [patch 1/2] ptrace, security: rename ptrace_may_access =>
	ptrace_access_check
Cc: Oleg Nesterov <oleg@redhat.com>, Roland McGrath <roland@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>

The ptrace_may_access() methods are named confusingly - some 
variants return a bool, while the security subsystem methods have a 
retval convention.

Rename it to ptrace_access_check, to reduce the confusion factor. A 
followup patch eliminates the bool usage.

[ Impact: cleanup, no code changed ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Roland McGrath <roland@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Oleg Nesterov <oleg@redhat.com>
LKML-Reference: <20090507084943.GB19133@elte.hu>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 fs/proc/array.c            |    2 +-
 fs/proc/base.c             |   10 +++++-----
 fs/proc/task_mmu.c         |    2 +-
 include/linux/ptrace.h     |    4 ++--
 include/linux/security.h   |   14 +++++++-------
 kernel/ptrace.c            |   10 +++++-----
 security/capability.c      |    2 +-
 security/commoncap.c       |    4 ++--
 security/root_plug.c       |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    6 +++---
 security/smack/smack_lsm.c |    8 ++++----
 12 files changed, 34 insertions(+), 34 deletions(-)

Index: linux/fs/proc/array.c
===================================================================
--- linux.orig/fs/proc/array.c
+++ linux/fs/proc/array.c
@@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	permitted = ptrace_may_access(task, PTRACE_MODE_READ);
+	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
Index: linux/fs/proc/base.c
===================================================================
--- linux.orig/fs/proc/base.c
+++ linux/fs/proc/base.c
@@ -222,7 +222,7 @@ static int check_mem_permission(struct t
 		rcu_read_lock();
 		match = (tracehook_tracer_task(task) == current);
 		rcu_read_unlock();
-		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
+		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
 			return 0;
 	}
 
@@ -242,7 +242,7 @@ struct mm_struct *mm_for_maps(struct tas
 	if (task->mm != mm)
 		goto out;
 	if (task->mm != current->mm &&
-	    __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
+	    __ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out;
 	task_unlock(task);
 	return mm;
@@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
 	wchan = get_wchan(task);
 
 	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
+		if (!ptrace_access_check(task, PTRACE_MODE_READ))
 			return 0;
 		else
 			return sprintf(buffer, "%lu", wchan);
@@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
 	 */
 	task = get_proc_task(inode);
 	if (task) {
-		allowed = ptrace_may_access(task, PTRACE_MODE_READ);
+		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
 		put_task_struct(task);
 	}
 	return allowed;
@@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
 	if (!task)
 		goto out_no_task;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_access_check(task, PTRACE_MODE_READ))
 		goto out;
 
 	ret = -ENOMEM;
Index: linux/fs/proc/task_mmu.c
===================================================================
--- linux.orig/fs/proc/task_mmu.c
+++ linux/fs/proc/task_mmu.c
@@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_access_check(task, PTRACE_MODE_READ))
 		goto out_task;
 
 	ret = -EINVAL;
Index: linux/include/linux/ptrace.h
===================================================================
--- linux.orig/include/linux/ptrace.h
+++ linux/include/linux/ptrace.h
@@ -99,9 +99,9 @@ extern void ptrace_fork(struct task_stru
 #define PTRACE_MODE_READ   1
 #define PTRACE_MODE_ATTACH 2
 /* Returns 0 on success, -errno on denial. */
-extern int __ptrace_may_access(struct task_struct *task, unsigned int mode);
+extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
 /* Returns true on success, false on denial. */
-extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
+extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
Index: linux/include/linux/security.h
===================================================================
--- linux.orig/include/linux/security.h
+++ linux/include/linux/security.h
@@ -52,7 +52,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1798,10 +1798,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
Index: linux/kernel/ptrace.c
===================================================================
--- linux.orig/kernel/ptrace.c
+++ linux/kernel/ptrace.c
@@ -127,7 +127,7 @@ int ptrace_check_attach(struct task_stru
 	return ret;
 }
 
-int __ptrace_may_access(struct task_struct *task, unsigned int mode)
+int __ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
 
@@ -162,14 +162,14 @@ int __ptrace_may_access(struct task_stru
 	if (!dumpable && !capable(CAP_SYS_PTRACE))
 		return -EPERM;
 
-	return security_ptrace_may_access(task, mode);
+	return security_ptrace_access_check(task, mode);
 }
 
-bool ptrace_may_access(struct task_struct *task, unsigned int mode)
+bool ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	int err;
 	task_lock(task);
-	err = __ptrace_may_access(task, mode);
+	err = __ptrace_access_check(task, mode);
 	task_unlock(task);
 	return !err;
 }
@@ -217,7 +217,7 @@ repeat:
 	/* the same process cannot be attached many times */
 	if (task->ptrace & PT_PTRACED)
 		goto bad;
-	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+	retval = __ptrace_access_check(task, PTRACE_MODE_ATTACH);
 	if (retval)
 		goto bad;
 
Index: linux/security/capability.c
===================================================================
--- linux.orig/security/capability.c
+++ linux/security/capability.c
@@ -863,7 +863,7 @@ struct security_operations default_secur
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
Index: linux/security/commoncap.c
===================================================================
--- linux.orig/security/commoncap.c
+++ linux/security/commoncap.c
@@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
Index: linux/security/root_plug.c
===================================================================
--- linux.orig/security/root_plug.c
+++ linux/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
Index: linux/security/security.c
===================================================================
--- linux.orig/security/security.c
+++ linux/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_op
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
Index: linux/security/selinux/hooks.c
===================================================================
--- linux.orig/security/selinux/hooks.c
+++ linux/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
Index: linux/security/smack/smack_lsm.c
===================================================================
--- linux.orig/security/smack/smack_lsm.c
+++ linux/security/smack/smack_lsm.c
@@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

----- End forwarded message -----
----- Forwarded message from Ingo Molnar <mingo@elte.hu> -----

Date: Thu, 7 May 2009 11:50:54 +0200
From: Ingo Molnar <mingo@elte.hu>
To: Chris Wright <chrisw@sous-sol.org>
Subject: [patch 2/2] ptrace: turn ptrace_access_check() into a retval
	function
Cc: Oleg Nesterov <oleg@redhat.com>, Roland McGrath <roland@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>

ptrace_access_check() returns a bool, while most of the ptrace 
access check machinery works with Linux retvals (where 0 indicates 
success, negative indicates an error).

So eliminate the bool and invert the usage at the call sites.

( Note: "< 0" checks are used instead of !0 checks, because that's
  the convention for retval checks and it results in similarly fast
  assembly code. )

[ Impact: cleanup ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 fs/proc/array.c        |    2 +-
 fs/proc/base.c         |    8 ++++----
 fs/proc/task_mmu.c     |    2 +-
 include/linux/ptrace.h |    2 +-
 kernel/ptrace.c        |    6 ++++--
 5 files changed, 11 insertions(+), 9 deletions(-)

Index: linux/fs/proc/array.c
===================================================================
--- linux.orig/fs/proc/array.c
+++ linux/fs/proc/array.c
@@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
+	permitted = !ptrace_access_check(task, PTRACE_MODE_READ);
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
Index: linux/fs/proc/base.c
===================================================================
--- linux.orig/fs/proc/base.c
+++ linux/fs/proc/base.c
@@ -222,7 +222,7 @@ static int check_mem_permission(struct t
 		rcu_read_lock();
 		match = (tracehook_tracer_task(task) == current);
 		rcu_read_unlock();
-		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
+		if (match && !ptrace_access_check(task, PTRACE_MODE_ATTACH))
 			return 0;
 	}
 
@@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
 	wchan = get_wchan(task);
 
 	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_access_check(task, PTRACE_MODE_READ))
+		if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 			return 0;
 		else
 			return sprintf(buffer, "%lu", wchan);
@@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
 	 */
 	task = get_proc_task(inode);
 	if (task) {
-		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
+		allowed = !ptrace_access_check(task, PTRACE_MODE_READ);
 		put_task_struct(task);
 	}
 	return allowed;
@@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
 	if (!task)
 		goto out_no_task;
 
-	if (!ptrace_access_check(task, PTRACE_MODE_READ))
+	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out;
 
 	ret = -ENOMEM;
Index: linux/fs/proc/task_mmu.c
===================================================================
--- linux.orig/fs/proc/task_mmu.c
+++ linux/fs/proc/task_mmu.c
@@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EACCES;
-	if (!ptrace_access_check(task, PTRACE_MODE_READ))
+	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out_task;
 
 	ret = -EINVAL;
Index: linux/include/linux/ptrace.h
===================================================================
--- linux.orig/include/linux/ptrace.h
+++ linux/include/linux/ptrace.h
@@ -101,7 +101,7 @@ extern void ptrace_fork(struct task_stru
 /* Returns 0 on success, -errno on denial. */
 extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
 /* Returns true on success, false on denial. */
-extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
+extern int ptrace_access_check(struct task_struct *task, unsigned int mode);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
Index: linux/kernel/ptrace.c
===================================================================
--- linux.orig/kernel/ptrace.c
+++ linux/kernel/ptrace.c
@@ -165,13 +165,15 @@ int __ptrace_access_check(struct task_st
 	return security_ptrace_access_check(task, mode);
 }
 
-bool ptrace_access_check(struct task_struct *task, unsigned int mode)
+int ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	int err;
+
 	task_lock(task);
 	err = __ptrace_access_check(task, mode);
 	task_unlock(task);
-	return !err;
+
+	return err;
 }
 
 int ptrace_attach(struct task_struct *task)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

----- End forwarded message -----

Re: security: rename ptrace_may_access => ptrace_access_check

[James Morris]

On Thu, 7 May 2009, Ingo Molnar wrote:

> > Boot tested with SELinux.
> 
> thanks! Below are the two patches i wrote and tested.

Do you need these in your tree, or are they ok going into linux-next via 
mine?


-- 
James Morris
<jmorris@namei.org>

Re: security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

* James Morris <jmorris@namei.org> wrote:

> On Thu, 7 May 2009, Ingo Molnar wrote:
> 
> > > Boot tested with SELinux.
> > 
> > thanks! Below are the two patches i wrote and tested.
> 
> Do you need these in your tree, or are they ok going into 
> linux-next via mine?

Sure, feel free to queue them up if everyone else is fine with them 
too.

	Ingo

Re: security: rename ptrace_may_access => ptrace_access_check

[Oleg Nesterov]

On 05/07, James Morris wrote:
>
> On Thu, 7 May 2009, Ingo Molnar wrote:
>
> > > Boot tested with SELinux.
> >
> > thanks! Below are the two patches i wrote and tested.
>
> Do you need these in your tree, or are they ok going into linux-next via
> mine?

James, Ingo, what happend with these renames? I don't see them
in linux/kernel/git/jmorris/security-testing-2.6.git.

I'd like to do some further cleanups on top of these changes,
or if they were dropped, against Linus's tree.

Oleg.

Re: security: rename ptrace_may_access => ptrace_access_check

[Christoph Hellwig]

Umm, I think I mentioned it before:  if you really need to rename that
beast please kill the ptrace part as we use it for all kinds of other
things.

[PATCH 0/1] mm_for_maps: simplify, use ptrace_may_access()

[Oleg Nesterov]

On 06/23, Christoph Hellwig wrote:
>
> Umm, I think I mentioned it before:  if you really need to rename that
> beast please kill the ptrace part as we use it for all kinds of other
> things.

Agreed, it would be nice to rename (and move out of ptrace.c) but I am
not going to do this, I can't suggest the better naming.

But I think it would be nice to kill __ptrace_may_access() and use
ptrace_may_access() everywhere. ptrace_attach() is trivial, the only
other caller is mm_for_maps().

Oleg.

[PATCH 1/1] mm_for_maps: simplify, use ptrace_may_access()

[Oleg Nesterov]

It would be nice to kill __ptrace_may_access(). It requires task_lock(),
but this lock is only needed to read mm->flags in the middle.

Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
the code a little bit.

Also, we do not need to take ->mmap_sem in advance. In fact I think
mm_for_maps() should not play with ->mmap_sem at all, the caller should
take this lock.

With or without this patch, without ->cred_guard_mutex held we can race
with exec() and get the new ->mm but check old creds.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- WAIT/fs/proc/base.c~1_MM_FOR_MAPS	2009-06-17 14:11:26.000000000 +0200
+++ WAIT/fs/proc/base.c	2009-06-23 20:16:44.000000000 +0200
@@ -237,20 +237,19 @@ struct mm_struct *mm_for_maps(struct tas
 	struct mm_struct *mm = get_task_mm(task);
 	if (!mm)
 		return NULL;
+	if (mm != current->mm) {
+		/*
+		 * task->mm can be changed before security check,
+		 * in that case we must notice the change after.
+		 */
+		if (!ptrace_may_access(task, PTRACE_MODE_READ) ||
+		    mm != task->mm) {
+			mmput(mm);
+			return NULL;
+		}
+	}
 	down_read(&mm->mmap_sem);
-	task_lock(task);
-	if (task->mm != mm)
-		goto out;
-	if (task->mm != current->mm &&
-	    __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
-		goto out;
-	task_unlock(task);
 	return mm;
-out:
-	task_unlock(task);
-	up_read(&mm->mmap_sem);
-	mmput(mm);
-	return NULL;
 }
 
 static int proc_pid_cmdline(struct task_struct *task, char * buffer)

Re: [PATCH 1/1] mm_for_maps: simplify, use ptrace_may_access()

[Serge E. Hallyn]

Quoting Oleg Nesterov (oleg@redhat.com):
> It would be nice to kill __ptrace_may_access(). It requires task_lock(),
> but this lock is only needed to read mm->flags in the middle.
> 
> Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
> the code a little bit.
> 
> Also, we do not need to take ->mmap_sem in advance. In fact I think
> mm_for_maps() should not play with ->mmap_sem at all, the caller should
> take this lock.

Yeah I think that makes sense.

> With or without this patch, without ->cred_guard_mutex held we can race
> with exec() and get the new ->mm but check old creds.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

reasoning on the security check also makes sense.

Reviewed-by: Serge Hallyn <serue@us.ibm.com>

> --- WAIT/fs/proc/base.c~1_MM_FOR_MAPS	2009-06-17 14:11:26.000000000 +0200
> +++ WAIT/fs/proc/base.c	2009-06-23 20:16:44.000000000 +0200
> @@ -237,20 +237,19 @@ struct mm_struct *mm_for_maps(struct tas
>  	struct mm_struct *mm = get_task_mm(task);
>  	if (!mm)
>  		return NULL;
> +	if (mm != current->mm) {
> +		/*
> +		 * task->mm can be changed before security check,
> +		 * in that case we must notice the change after.
> +		 */
> +		if (!ptrace_may_access(task, PTRACE_MODE_READ) ||
> +		    mm != task->mm) {
> +			mmput(mm);
> +			return NULL;
> +		}
> +	}
>  	down_read(&mm->mmap_sem);
> -	task_lock(task);
> -	if (task->mm != mm)
> -		goto out;
> -	if (task->mm != current->mm &&
> -	    __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
> -		goto out;
> -	task_unlock(task);
>  	return mm;
> -out:
> -	task_unlock(task);
> -	up_read(&mm->mmap_sem);
> -	mmput(mm);
> -	return NULL;
>  }
> 
>  static int proc_pid_cmdline(struct task_struct *task, char * buffer)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] mm_for_maps: simplify, use ptrace_may_access()

[James Morris]

On Tue, 23 Jun 2009, Serge E. Hallyn wrote:

> Quoting Oleg Nesterov (oleg@redhat.com):
> > It would be nice to kill __ptrace_may_access(). It requires task_lock(),
> > but this lock is only needed to read mm->flags in the middle.
> > 
> > Convert mm_for_maps() to use ptrace_may_access(), this also simplifies
> > the code a little bit.
> > 
> > Also, we do not need to take ->mmap_sem in advance. In fact I think
> > mm_for_maps() should not play with ->mmap_sem at all, the caller should
> > take this lock.
> 
> Yeah I think that makes sense.
> 
> > With or without this patch, without ->cred_guard_mutex held we can race
> > with exec() and get the new ->mm but check old creds.
> > 
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> 
> reasoning on the security check also makes sense.
> 
> Reviewed-by: Serge Hallyn <serue@us.ibm.com>


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


-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 1/1] mm_for_maps: simplify, use ptrace_may_access()

[Roland McGrath]

> Also, we do not need to take ->mmap_sem in advance. In fact I think
> mm_for_maps() should not play with ->mmap_sem at all, the caller should
> take this lock.

Agreed.  It has only one caller (though two forks of it) in
fs/proc/task_{no,}mmu.c and it looks easy to change.

> With or without this patch, without ->cred_guard_mutex held we can race
> with exec() and get the new ->mm but check old creds.

It looks safe and proper for mm_for_maps() to take that mutex around its
check.  Your patch looks good to me as it is, and taking cred_guard_mutex
can be another security fix patch on top.


Thanks,
Roland

Re: [PATCH 1/1] mm_for_maps: simplify, use ptrace_may_access()

[Oleg Nesterov]

On 06/24, Roland McGrath wrote:
>
> > Also, we do not need to take ->mmap_sem in advance. In fact I think
> > mm_for_maps() should not play with ->mmap_sem at all, the caller should
> > take this lock.
>
> Agreed.  It has only one caller (though two forks of it) in
> fs/proc/task_{no,}mmu.c and it looks easy to change.
>
> > With or without this patch, without ->cred_guard_mutex held we can race
> > with exec() and get the new ->mm but check old creds.
>
> It looks safe and proper for mm_for_maps() to take that mutex around its
> check.  Your patch looks good to me as it is, and taking cred_guard_mutex
> can be another security fix patch on top.

Agreed, will do.

Oleg.

Re: security: rename ptrace_may_access => ptrace_access_check

[James Morris]

On Tue, 23 Jun 2009, Oleg Nesterov wrote:

> On 05/07, James Morris wrote:
> >
> > On Thu, 7 May 2009, Ingo Molnar wrote:
> >
> > > > Boot tested with SELinux.
> > >
> > > thanks! Below are the two patches i wrote and tested.
> >
> > Do you need these in your tree, or are they ok going into linux-next via
> > mine?
> 
> James, Ingo, what happend with these renames? I don't see them
> in linux/kernel/git/jmorris/security-testing-2.6.git.

The context was probably lost for some reason, e.g. waiting on acks or for 
reviewer questions to be resolved.  Please resend if you don't see them 
applied and also always indicate any depdendencies on other patches.

It is unliklely that I'd apply security-related patches of any kind of 
non-trivial nature without reviewed-by or acked-by lines from other 
developers, and you may need to engage reviewers on an individual basis to 
make that happen.


- James
-- 
James Morris
<jmorris@namei.org>

Re: your mail

[Casey Schaufler]

Ingo Molnar wrote:
> * James Morris <jmorris@namei.org> wrote:
>
>   
>> On Thu, 7 May 2009, Chris Wright wrote:
>>
>>     
>>> * Ingo Molnar (mingo@elte.hu) wrote:
>>>       
>> [Added LSM list to the CC; please do so whenever making changes in this 
>> area...]
>>
>>     
>>>> They have no active connection to the core kernel 
>>>> ptrace_may_access() check in any case:
>>>>         
>>> Not sure what you mean:
>>>
>>> ptrace_may_access
>>>  __ptrace_may_access
>>>   security_ptrace_may_access
>>>
>>> Looks like your patch won't compile.
>>>
>>>       
>> Below is an updated version which fixes the bug, against 
>> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
>>
>> Boot tested with SELinux.
>>     
>
> thanks! Below are the two patches i wrote and tested.
>   

I hate to make an assumption regarding whether or not your tests
included Smack, so I'll ask. Does tested mean with Smack?

Thank you.

> 	Ingo
>
> ----- Forwarded message from Ingo Molnar <mingo@elte.hu> -----
>
> Date: Thu, 7 May 2009 11:49:47 +0200
> From: Ingo Molnar <mingo@elte.hu>
> To: Chris Wright <chrisw@sous-sol.org>
> Subject: [patch 1/2] ptrace, security: rename ptrace_may_access =>
> 	ptrace_access_check
> Cc: Oleg Nesterov <oleg@redhat.com>, Roland McGrath <roland@redhat.com>,
> 	Andrew Morton <akpm@linux-foundation.org>,
> 	linux-kernel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>
>
> The ptrace_may_access() methods are named confusingly - some 
> variants return a bool, while the security subsystem methods have a 
> retval convention.
>
> Rename it to ptrace_access_check, to reduce the confusion factor. A 
> followup patch eliminates the bool usage.
>
> [ Impact: cleanup, no code changed ]
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Cc: Roland McGrath <roland@redhat.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Chris Wright <chrisw@sous-sol.org>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Cc: Oleg Nesterov <oleg@redhat.com>
> LKML-Reference: <20090507084943.GB19133@elte.hu>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  fs/proc/array.c            |    2 +-
>  fs/proc/base.c             |   10 +++++-----
>  fs/proc/task_mmu.c         |    2 +-
>  include/linux/ptrace.h     |    4 ++--
>  include/linux/security.h   |   14 +++++++-------
>  kernel/ptrace.c            |   10 +++++-----
>  security/capability.c      |    2 +-
>  security/commoncap.c       |    4 ++--
>  security/root_plug.c       |    2 +-
>  security/security.c        |    4 ++--
>  security/selinux/hooks.c   |    6 +++---
>  security/smack/smack_lsm.c |    8 ++++----
>  12 files changed, 34 insertions(+), 34 deletions(-)
>
> Index: linux/fs/proc/array.c
> ===================================================================
> --- linux.orig/fs/proc/array.c
> +++ linux/fs/proc/array.c
> @@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
>  
>  	state = *get_task_state(task);
>  	vsize = eip = esp = 0;
> -	permitted = ptrace_may_access(task, PTRACE_MODE_READ);
> +	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
>  	mm = get_task_mm(task);
>  	if (mm) {
>  		vsize = task_vsize(mm);
> Index: linux/fs/proc/base.c
> ===================================================================
> --- linux.orig/fs/proc/base.c
> +++ linux/fs/proc/base.c
> @@ -222,7 +222,7 @@ static int check_mem_permission(struct t
>  		rcu_read_lock();
>  		match = (tracehook_tracer_task(task) == current);
>  		rcu_read_unlock();
> -		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
> +		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
>  			return 0;
>  	}
>  
> @@ -242,7 +242,7 @@ struct mm_struct *mm_for_maps(struct tas
>  	if (task->mm != mm)
>  		goto out;
>  	if (task->mm != current->mm &&
> -	    __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
> +	    __ptrace_access_check(task, PTRACE_MODE_READ) < 0)
>  		goto out;
>  	task_unlock(task);
>  	return mm;
> @@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
>  	wchan = get_wchan(task);
>  
>  	if (lookup_symbol_name(wchan, symname) < 0)
> -		if (!ptrace_may_access(task, PTRACE_MODE_READ))
> +		if (!ptrace_access_check(task, PTRACE_MODE_READ))
>  			return 0;
>  		else
>  			return sprintf(buffer, "%lu", wchan);
> @@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
>  	 */
>  	task = get_proc_task(inode);
>  	if (task) {
> -		allowed = ptrace_may_access(task, PTRACE_MODE_READ);
> +		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
>  		put_task_struct(task);
>  	}
>  	return allowed;
> @@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
>  	if (!task)
>  		goto out_no_task;
>  
> -	if (!ptrace_may_access(task, PTRACE_MODE_READ))
> +	if (!ptrace_access_check(task, PTRACE_MODE_READ))
>  		goto out;
>  
>  	ret = -ENOMEM;
> Index: linux/fs/proc/task_mmu.c
> ===================================================================
> --- linux.orig/fs/proc/task_mmu.c
> +++ linux/fs/proc/task_mmu.c
> @@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
>  		goto out;
>  
>  	ret = -EACCES;
> -	if (!ptrace_may_access(task, PTRACE_MODE_READ))
> +	if (!ptrace_access_check(task, PTRACE_MODE_READ))
>  		goto out_task;
>  
>  	ret = -EINVAL;
> Index: linux/include/linux/ptrace.h
> ===================================================================
> --- linux.orig/include/linux/ptrace.h
> +++ linux/include/linux/ptrace.h
> @@ -99,9 +99,9 @@ extern void ptrace_fork(struct task_stru
>  #define PTRACE_MODE_READ   1
>  #define PTRACE_MODE_ATTACH 2
>  /* Returns 0 on success, -errno on denial. */
> -extern int __ptrace_may_access(struct task_struct *task, unsigned int mode);
> +extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
>  /* Returns true on success, false on denial. */
> -extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
> +extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
>  
>  static inline int ptrace_reparented(struct task_struct *child)
>  {
> Index: linux/include/linux/security.h
> ===================================================================
> --- linux.orig/include/linux/security.h
> +++ linux/include/linux/security.h
> @@ -52,7 +52,7 @@ struct audit_krule;
>  extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
>  		       int cap, int audit);
>  extern int cap_settime(struct timespec *ts, struct timezone *tz);
> -extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
> +extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  extern int cap_ptrace_traceme(struct task_struct *parent);
>  extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
>  extern int cap_capset(struct cred *new, const struct cred *old,
> @@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
>   *	@alter contains the flag indicating whether changes are to be made.
>   *	Return 0 if permission is granted.
>   *
> - * @ptrace_may_access:
> + * @ptrace_access_check:
>   *	Check permission before allowing the current process to trace the
>   *	@child process.
>   *	Security modules may also want to perform a process tracing check
> @@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
>   *	Check that the @parent process has sufficient permission to trace the
>   *	current process before allowing the current process to present itself
>   *	to the @parent process for tracing.
> - *	The parent process will still have to undergo the ptrace_may_access
> + *	The parent process will still have to undergo the ptrace_access_check
>   *	checks before it is allowed to trace this one.
>   *	@parent contains the task_struct structure for debugger process.
>   *	Return 0 if permission is granted.
> @@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
>  struct security_operations {
>  	char name[SECURITY_NAME_MAX + 1];
>  
> -	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
> +	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
>  	int (*ptrace_traceme) (struct task_struct *parent);
>  	int (*capget) (struct task_struct *target,
>  		       kernel_cap_t *effective,
> @@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
>  extern int register_security(struct security_operations *ops);
>  
>  /* Security operations */
> -int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
> +int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  int security_ptrace_traceme(struct task_struct *parent);
>  int security_capget(struct task_struct *target,
>  		    kernel_cap_t *effective,
> @@ -1798,10 +1798,10 @@ static inline int security_init(void)
>  	return 0;
>  }
>  
> -static inline int security_ptrace_may_access(struct task_struct *child,
> +static inline int security_ptrace_access_check(struct task_struct *child,
>  					     unsigned int mode)
>  {
> -	return cap_ptrace_may_access(child, mode);
> +	return cap_ptrace_access_check(child, mode);
>  }
>  
>  static inline int security_ptrace_traceme(struct task_struct *parent)
> Index: linux/kernel/ptrace.c
> ===================================================================
> --- linux.orig/kernel/ptrace.c
> +++ linux/kernel/ptrace.c
> @@ -127,7 +127,7 @@ int ptrace_check_attach(struct task_stru
>  	return ret;
>  }
>  
> -int __ptrace_may_access(struct task_struct *task, unsigned int mode)
> +int __ptrace_access_check(struct task_struct *task, unsigned int mode)
>  {
>  	const struct cred *cred = current_cred(), *tcred;
>  
> @@ -162,14 +162,14 @@ int __ptrace_may_access(struct task_stru
>  	if (!dumpable && !capable(CAP_SYS_PTRACE))
>  		return -EPERM;
>  
> -	return security_ptrace_may_access(task, mode);
> +	return security_ptrace_access_check(task, mode);
>  }
>  
> -bool ptrace_may_access(struct task_struct *task, unsigned int mode)
> +bool ptrace_access_check(struct task_struct *task, unsigned int mode)
>  {
>  	int err;
>  	task_lock(task);
> -	err = __ptrace_may_access(task, mode);
> +	err = __ptrace_access_check(task, mode);
>  	task_unlock(task);
>  	return !err;
>  }
> @@ -217,7 +217,7 @@ repeat:
>  	/* the same process cannot be attached many times */
>  	if (task->ptrace & PT_PTRACED)
>  		goto bad;
> -	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
> +	retval = __ptrace_access_check(task, PTRACE_MODE_ATTACH);
>  	if (retval)
>  		goto bad;
>  
> Index: linux/security/capability.c
> ===================================================================
> --- linux.orig/security/capability.c
> +++ linux/security/capability.c
> @@ -863,7 +863,7 @@ struct security_operations default_secur
>  
>  void security_fixup_ops(struct security_operations *ops)
>  {
> -	set_to_cap_if_null(ops, ptrace_may_access);
> +	set_to_cap_if_null(ops, ptrace_access_check);
>  	set_to_cap_if_null(ops, ptrace_traceme);
>  	set_to_cap_if_null(ops, capget);
>  	set_to_cap_if_null(ops, capset);
> Index: linux/security/commoncap.c
> ===================================================================
> --- linux.orig/security/commoncap.c
> +++ linux/security/commoncap.c
> @@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
>  }
>  
>  /**
> - * cap_ptrace_may_access - Determine whether the current process may access
> + * cap_ptrace_access_check - Determine whether the current process may access
>   *			   another
>   * @child: The process to be accessed
>   * @mode: The mode of attachment.
> @@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
>   * Determine whether a process may access another, returning 0 if permission
>   * granted, -ve if denied.
>   */
> -int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
> +int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
>  {
>  	int ret = 0;
>  
> Index: linux/security/root_plug.c
> ===================================================================
> --- linux.orig/security/root_plug.c
> +++ linux/security/root_plug.c
> @@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
>  
>  static struct security_operations rootplug_security_ops = {
>  	/* Use the capability functions for some of the hooks */
> -	.ptrace_may_access =		cap_ptrace_may_access,
> +	.ptrace_access_check =		cap_ptrace_access_check,
>  	.ptrace_traceme =		cap_ptrace_traceme,
>  	.capget =			cap_capget,
>  	.capset =			cap_capset,
> Index: linux/security/security.c
> ===================================================================
> --- linux.orig/security/security.c
> +++ linux/security/security.c
> @@ -127,9 +127,9 @@ int register_security(struct security_op
>  
>  /* Security operations */
>  
> -int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
> +int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
>  {
> -	return security_ops->ptrace_may_access(child, mode);
> +	return security_ops->ptrace_access_check(child, mode);
>  }
>  
>  int security_ptrace_traceme(struct task_struct *parent)
> Index: linux/security/selinux/hooks.c
> ===================================================================
> --- linux.orig/security/selinux/hooks.c
> +++ linux/security/selinux/hooks.c
> @@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
>  
>  /* Hook functions begin here. */
>  
> -static int selinux_ptrace_may_access(struct task_struct *child,
> +static int selinux_ptrace_access_check(struct task_struct *child,
>  				     unsigned int mode)
>  {
>  	int rc;
>  
> -	rc = cap_ptrace_may_access(child, mode);
> +	rc = cap_ptrace_access_check(child, mode);
>  	if (rc)
>  		return rc;
>  
> @@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
>  static struct security_operations selinux_ops = {
>  	.name =				"selinux",
>  
> -	.ptrace_may_access =		selinux_ptrace_may_access,
> +	.ptrace_access_check =		selinux_ptrace_access_check,
>  	.ptrace_traceme =		selinux_ptrace_traceme,
>  	.capget =			selinux_capget,
>  	.capset =			selinux_capset,
> Index: linux/security/smack/smack_lsm.c
> ===================================================================
> --- linux.orig/security/smack/smack_lsm.c
> +++ linux/security/smack/smack_lsm.c
> @@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
>   */
>  
>  /**
> - * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
> + * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
>   * @ctp: child task pointer
>   * @mode: ptrace attachment mode
>   *
> @@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
>   *
>   * Do the capability checks, and require read and write.
>   */
> -static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
> +static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>  {
>  	int rc;
>  
> -	rc = cap_ptrace_may_access(ctp, mode);
> +	rc = cap_ptrace_access_check(ctp, mode);
>  	if (rc != 0)
>  		return rc;
>  
> @@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
>  struct security_operations smack_ops = {
>  	.name =				"smack",
>  
> -	.ptrace_may_access =		smack_ptrace_may_access,
> +	.ptrace_access_check =		smack_ptrace_access_check,
>  	.ptrace_traceme =		smack_ptrace_traceme,
>  	.capget = 			cap_capget,
>  	.capset = 			cap_capset,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
> ----- End forwarded message -----
> ----- Forwarded message from Ingo Molnar <mingo@elte.hu> -----
>
> Date: Thu, 7 May 2009 11:50:54 +0200
> From: Ingo Molnar <mingo@elte.hu>
> To: Chris Wright <chrisw@sous-sol.org>
> Subject: [patch 2/2] ptrace: turn ptrace_access_check() into a retval
> 	function
> Cc: Oleg Nesterov <oleg@redhat.com>, Roland McGrath <roland@redhat.com>,
> 	Andrew Morton <akpm@linux-foundation.org>,
> 	linux-kernel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>
>
> ptrace_access_check() returns a bool, while most of the ptrace 
> access check machinery works with Linux retvals (where 0 indicates 
> success, negative indicates an error).
>
> So eliminate the bool and invert the usage at the call sites.
>
> ( Note: "< 0" checks are used instead of !0 checks, because that's
>   the convention for retval checks and it results in similarly fast
>   assembly code. )
>
> [ Impact: cleanup ]
>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  fs/proc/array.c        |    2 +-
>  fs/proc/base.c         |    8 ++++----
>  fs/proc/task_mmu.c     |    2 +-
>  include/linux/ptrace.h |    2 +-
>  kernel/ptrace.c        |    6 ++++--
>  5 files changed, 11 insertions(+), 9 deletions(-)
>
> Index: linux/fs/proc/array.c
> ===================================================================
> --- linux.orig/fs/proc/array.c
> +++ linux/fs/proc/array.c
> @@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
>  
>  	state = *get_task_state(task);
>  	vsize = eip = esp = 0;
> -	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
> +	permitted = !ptrace_access_check(task, PTRACE_MODE_READ);
>  	mm = get_task_mm(task);
>  	if (mm) {
>  		vsize = task_vsize(mm);
> Index: linux/fs/proc/base.c
> ===================================================================
> --- linux.orig/fs/proc/base.c
> +++ linux/fs/proc/base.c
> @@ -222,7 +222,7 @@ static int check_mem_permission(struct t
>  		rcu_read_lock();
>  		match = (tracehook_tracer_task(task) == current);
>  		rcu_read_unlock();
> -		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
> +		if (match && !ptrace_access_check(task, PTRACE_MODE_ATTACH))
>  			return 0;
>  	}
>  
> @@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
>  	wchan = get_wchan(task);
>  
>  	if (lookup_symbol_name(wchan, symname) < 0)
> -		if (!ptrace_access_check(task, PTRACE_MODE_READ))
> +		if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
>  			return 0;
>  		else
>  			return sprintf(buffer, "%lu", wchan);
> @@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
>  	 */
>  	task = get_proc_task(inode);
>  	if (task) {
> -		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
> +		allowed = !ptrace_access_check(task, PTRACE_MODE_READ);
>  		put_task_struct(task);
>  	}
>  	return allowed;
> @@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
>  	if (!task)
>  		goto out_no_task;
>  
> -	if (!ptrace_access_check(task, PTRACE_MODE_READ))
> +	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
>  		goto out;
>  
>  	ret = -ENOMEM;
> Index: linux/fs/proc/task_mmu.c
> ===================================================================
> --- linux.orig/fs/proc/task_mmu.c
> +++ linux/fs/proc/task_mmu.c
> @@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
>  		goto out;
>  
>  	ret = -EACCES;
> -	if (!ptrace_access_check(task, PTRACE_MODE_READ))
> +	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
>  		goto out_task;
>  
>  	ret = -EINVAL;
> Index: linux/include/linux/ptrace.h
> ===================================================================
> --- linux.orig/include/linux/ptrace.h
> +++ linux/include/linux/ptrace.h
> @@ -101,7 +101,7 @@ extern void ptrace_fork(struct task_stru
>  /* Returns 0 on success, -errno on denial. */
>  extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
>  /* Returns true on success, false on denial. */
> -extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
> +extern int ptrace_access_check(struct task_struct *task, unsigned int mode);
>  
>  static inline int ptrace_reparented(struct task_struct *child)
>  {
> Index: linux/kernel/ptrace.c
> ===================================================================
> --- linux.orig/kernel/ptrace.c
> +++ linux/kernel/ptrace.c
> @@ -165,13 +165,15 @@ int __ptrace_access_check(struct task_st
>  	return security_ptrace_access_check(task, mode);
>  }
>  
> -bool ptrace_access_check(struct task_struct *task, unsigned int mode)
> +int ptrace_access_check(struct task_struct *task, unsigned int mode)
>  {
>  	int err;
> +
>  	task_lock(task);
>  	err = __ptrace_access_check(task, mode);
>  	task_unlock(task);
> -	return !err;
> +
> +	return err;
>  }
>  
>  int ptrace_attach(struct task_struct *task)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
> ----- End forwarded message -----
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>   

Re: security: rename ptrace_may_access => ptrace_access_check

[James Morris]

This has now been applied to

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


On Thu, 7 May 2009, James Morris wrote:

> On Thu, 7 May 2009, Chris Wright wrote:
> 
> > * Ingo Molnar (mingo@elte.hu) wrote:
> 
> [Added LSM list to the CC; please do so whenever making changes in this 
> area...]
> 
> > > They have no active connection to the core kernel 
> > > ptrace_may_access() check in any case:
> > 
> > Not sure what you mean:
> > 
> > ptrace_may_access
> >  __ptrace_may_access
> >   security_ptrace_may_access
> > 
> > Looks like your patch won't compile.
> > 
> 
> Below is an updated version which fixes the bug, against 
> git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next
> 
> Boot tested with SELinux.
> 
> commit c4c79671177dc3e8387c337f75f3c664cdf08838
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Thu May 7 19:26:19 2009 +1000
> 
>     security: rename ptrace_may_access => ptrace_access_check
>     
>     The ->ptrace_may_access() methods are named confusingly - the real
>     ptrace_may_access() returns a bool, while these security checks have
>     a retval convention.
>     
>     Rename it to ptrace_access_check, to reduce the confusion factor.
>     
>     [ Impact: cleanup, no code changed ]
>     
>     Signed-off-by: Ingo Molnar <mingo@elte.hu>
>     Signed-off-by: James Morris <jmorris@namei.org>
> 
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 54ed157..0147def 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -51,7 +51,7 @@ struct audit_krule;
>  extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
>  		       int cap, int audit);
>  extern int cap_settime(struct timespec *ts, struct timezone *tz);
> -extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
> +extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  extern int cap_ptrace_traceme(struct task_struct *parent);
>  extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
>  extern int cap_capset(struct cred *new, const struct cred *old,
> @@ -1208,7 +1208,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	@alter contains the flag indicating whether changes are to be made.
>   *	Return 0 if permission is granted.
>   *
> - * @ptrace_may_access:
> + * @ptrace_access_check:
>   *	Check permission before allowing the current process to trace the
>   *	@child process.
>   *	Security modules may also want to perform a process tracing check
> @@ -1223,7 +1223,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>   *	Check that the @parent process has sufficient permission to trace the
>   *	current process before allowing the current process to present itself
>   *	to the @parent process for tracing.
> - *	The parent process will still have to undergo the ptrace_may_access
> + *	The parent process will still have to undergo the ptrace_access_check
>   *	checks before it is allowed to trace this one.
>   *	@parent contains the task_struct structure for debugger process.
>   *	Return 0 if permission is granted.
> @@ -1335,7 +1335,7 @@ static inline void security_free_mnt_opts(struct security_mnt_opts *opts)
>  struct security_operations {
>  	char name[SECURITY_NAME_MAX + 1];
>  
> -	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
> +	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
>  	int (*ptrace_traceme) (struct task_struct *parent);
>  	int (*capget) (struct task_struct *target,
>  		       kernel_cap_t *effective,
> @@ -1616,7 +1616,7 @@ extern int security_module_enable(struct security_operations *ops);
>  extern int register_security(struct security_operations *ops);
>  
>  /* Security operations */
> -int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
> +int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
>  int security_ptrace_traceme(struct task_struct *parent);
>  int security_capget(struct task_struct *target,
>  		    kernel_cap_t *effective,
> @@ -1797,10 +1797,10 @@ static inline int security_init(void)
>  	return 0;
>  }
>  
> -static inline int security_ptrace_may_access(struct task_struct *child,
> +static inline int security_ptrace_access_check(struct task_struct *child,
>  					     unsigned int mode)
>  {
> -	return cap_ptrace_may_access(child, mode);
> +	return cap_ptrace_access_check(child, mode);
>  }
>  
>  static inline int security_ptrace_traceme(struct task_struct *parent)
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index c9cf48b..284d0ac 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -160,7 +160,7 @@ int __ptrace_may_access(struct task_struct *task, unsigned int mode)
>  	if (!dumpable && !capable(CAP_SYS_PTRACE))
>  		return -EPERM;
>  
> -	return security_ptrace_may_access(task, mode);
> +	return security_ptrace_access_check(task, mode);
>  }
>  
>  bool ptrace_may_access(struct task_struct *task, unsigned int mode)
> diff --git a/security/capability.c b/security/capability.c
> index 21b6cea..f218dd3 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -863,7 +863,7 @@ struct security_operations default_security_ops = {
>  
>  void security_fixup_ops(struct security_operations *ops)
>  {
> -	set_to_cap_if_null(ops, ptrace_may_access);
> +	set_to_cap_if_null(ops, ptrace_access_check);
>  	set_to_cap_if_null(ops, ptrace_traceme);
>  	set_to_cap_if_null(ops, capget);
>  	set_to_cap_if_null(ops, capset);
> diff --git a/security/commoncap.c b/security/commoncap.c
> index 97ac1f1..e57611a 100644
> --- a/security/commoncap.c
> +++ b/security/commoncap.c
> @@ -101,7 +101,7 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
>  }
>  
>  /**
> - * cap_ptrace_may_access - Determine whether the current process may access
> + * cap_ptrace_access_check - Determine whether the current process may access
>   *			   another
>   * @child: The process to be accessed
>   * @mode: The mode of attachment.
> @@ -109,7 +109,7 @@ int cap_settime(struct timespec *ts, struct timezone *tz)
>   * Determine whether a process may access another, returning 0 if permission
>   * granted, -ve if denied.
>   */
> -int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
> +int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
>  {
>  	int ret = 0;
>  
> diff --git a/security/root_plug.c b/security/root_plug.c
> index 40fb4f1..e8d5861 100644
> --- a/security/root_plug.c
> +++ b/security/root_plug.c
> @@ -72,7 +72,7 @@ static int rootplug_bprm_check_security (struct linux_binprm *bprm)
>  
>  static struct security_operations rootplug_security_ops = {
>  	/* Use the capability functions for some of the hooks */
> -	.ptrace_may_access =		cap_ptrace_may_access,
> +	.ptrace_access_check =		cap_ptrace_access_check,
>  	.ptrace_traceme =		cap_ptrace_traceme,
>  	.capget =			cap_capget,
>  	.capset =			cap_capset,
> diff --git a/security/security.c b/security/security.c
> index 206e538..a3e6918 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -127,9 +127,9 @@ int register_security(struct security_operations *ops)
>  
>  /* Security operations */
>  
> -int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
> +int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
>  {
> -	return security_ops->ptrace_may_access(child, mode);
> +	return security_ops->ptrace_access_check(child, mode);
>  }
>  
>  int security_ptrace_traceme(struct task_struct *parent)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 39046dd..e30c4bb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct file *file)
>  
>  /* Hook functions begin here. */
>  
> -static int selinux_ptrace_may_access(struct task_struct *child,
> +static int selinux_ptrace_access_check(struct task_struct *child,
>  				     unsigned int mode)
>  {
>  	int rc;
>  
> -	rc = cap_ptrace_may_access(child, mode);
> +	rc = cap_ptrace_access_check(child, mode);
>  	if (rc)
>  		return rc;
>  
> @@ -5310,7 +5310,7 @@ static int selinux_key_getsecurity(struct key *key, char **_buffer)
>  static struct security_operations selinux_ops = {
>  	.name =				"selinux",
>  
> -	.ptrace_may_access =		selinux_ptrace_may_access,
> +	.ptrace_access_check =		selinux_ptrace_access_check,
>  	.ptrace_traceme =		selinux_ptrace_traceme,
>  	.capget =			selinux_capget,
>  	.capset =			selinux_capset,
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index f557767..79949f9 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -91,7 +91,7 @@ struct inode_smack *new_inode_smack(char *smack)
>   */
>  
>  /**
> - * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
> + * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
>   * @ctp: child task pointer
>   * @mode: ptrace attachment mode
>   *
> @@ -99,13 +99,13 @@ struct inode_smack *new_inode_smack(char *smack)
>   *
>   * Do the capability checks, and require read and write.
>   */
> -static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
> +static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
>  {
>  	int rc;
>  	struct smk_audit_info ad;
>  	char *sp, *tsp;
>  
> -	rc = cap_ptrace_may_access(ctp, mode);
> +	rc = cap_ptrace_access_check(ctp, mode);
>  	if (rc != 0)
>  		return rc;
>  
> @@ -3031,7 +3031,7 @@ static void smack_release_secctx(char *secdata, u32 seclen)
>  struct security_operations smack_ops = {
>  	.name =				"smack",
>  
> -	.ptrace_may_access =		smack_ptrace_may_access,
> +	.ptrace_access_check =		smack_ptrace_access_check,
>  	.ptrace_traceme =		smack_ptrace_traceme,
>  	.capget = 			cap_capget,
>  	.capset = 			cap_capset,
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
James Morris
<jmorris@namei.org>

Re: [RFC PATCH 3/3a] ptrace: add _ptrace_may_access()

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> The design around that code does not seem to be very consistent.
> 
> One solution would be for the default "plain Linux" security 
> module to have a stock ->ptrace_access_check() that does the 
> current ptrace_may_access() check, and then procfs could be 
> updated to use that callback - instead of calling into the ptrace 
> core code directly.

hm, that's not a good idea, as we'd have an unnecessary indirect 
call even in the common case where the higher-level ptrace checks 
deny a request via -EPERM early on already.

So it's all designed fine and what we need is the rename plus the 
elimination of the bool.

	Ingo

[patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

The ptrace_may_access() methods are named confusingly - some 
variants return a bool, while the security subsystem methods have a 
retval convention.

Rename it to ptrace_access_check, to reduce the confusion factor. A 
followup patch eliminates the bool usage.

[ Impact: cleanup, no code changed ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Roland McGrath <roland@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Chris Wright <chrisw@sous-sol.org>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Oleg Nesterov <oleg@redhat.com>
LKML-Reference: <20090507084943.GB19133@elte.hu>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 fs/proc/array.c            |    2 +-
 fs/proc/base.c             |   10 +++++-----
 fs/proc/task_mmu.c         |    2 +-
 include/linux/ptrace.h     |    4 ++--
 include/linux/security.h   |   14 +++++++-------
 kernel/ptrace.c            |   10 +++++-----
 security/capability.c      |    2 +-
 security/commoncap.c       |    4 ++--
 security/root_plug.c       |    2 +-
 security/security.c        |    4 ++--
 security/selinux/hooks.c   |    6 +++---
 security/smack/smack_lsm.c |    8 ++++----
 12 files changed, 34 insertions(+), 34 deletions(-)

Index: linux/fs/proc/array.c
===================================================================
--- linux.orig/fs/proc/array.c
+++ linux/fs/proc/array.c
@@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	permitted = ptrace_may_access(task, PTRACE_MODE_READ);
+	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
Index: linux/fs/proc/base.c
===================================================================
--- linux.orig/fs/proc/base.c
+++ linux/fs/proc/base.c
@@ -222,7 +222,7 @@ static int check_mem_permission(struct t
 		rcu_read_lock();
 		match = (tracehook_tracer_task(task) == current);
 		rcu_read_unlock();
-		if (match && ptrace_may_access(task, PTRACE_MODE_ATTACH))
+		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
 			return 0;
 	}
 
@@ -242,7 +242,7 @@ struct mm_struct *mm_for_maps(struct tas
 	if (task->mm != mm)
 		goto out;
 	if (task->mm != current->mm &&
-	    __ptrace_may_access(task, PTRACE_MODE_READ) < 0)
+	    __ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out;
 	task_unlock(task);
 	return mm;
@@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
 	wchan = get_wchan(task);
 
 	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_may_access(task, PTRACE_MODE_READ))
+		if (!ptrace_access_check(task, PTRACE_MODE_READ))
 			return 0;
 		else
 			return sprintf(buffer, "%lu", wchan);
@@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
 	 */
 	task = get_proc_task(inode);
 	if (task) {
-		allowed = ptrace_may_access(task, PTRACE_MODE_READ);
+		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
 		put_task_struct(task);
 	}
 	return allowed;
@@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
 	if (!task)
 		goto out_no_task;
 
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_access_check(task, PTRACE_MODE_READ))
 		goto out;
 
 	ret = -ENOMEM;
Index: linux/fs/proc/task_mmu.c
===================================================================
--- linux.orig/fs/proc/task_mmu.c
+++ linux/fs/proc/task_mmu.c
@@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EACCES;
-	if (!ptrace_may_access(task, PTRACE_MODE_READ))
+	if (!ptrace_access_check(task, PTRACE_MODE_READ))
 		goto out_task;
 
 	ret = -EINVAL;
Index: linux/include/linux/ptrace.h
===================================================================
--- linux.orig/include/linux/ptrace.h
+++ linux/include/linux/ptrace.h
@@ -99,9 +99,9 @@ extern void ptrace_fork(struct task_stru
 #define PTRACE_MODE_READ   1
 #define PTRACE_MODE_ATTACH 2
 /* Returns 0 on success, -errno on denial. */
-extern int __ptrace_may_access(struct task_struct *task, unsigned int mode);
+extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
 /* Returns true on success, false on denial. */
-extern bool ptrace_may_access(struct task_struct *task, unsigned int mode);
+extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
Index: linux/include/linux/security.h
===================================================================
--- linux.orig/include/linux/security.h
+++ linux/include/linux/security.h
@@ -52,7 +52,7 @@ struct audit_krule;
 extern int cap_capable(struct task_struct *tsk, const struct cred *cred,
 		       int cap, int audit);
 extern int cap_settime(struct timespec *ts, struct timezone *tz);
-extern int cap_ptrace_may_access(struct task_struct *child, unsigned int mode);
+extern int cap_ptrace_access_check(struct task_struct *child, unsigned int mode);
 extern int cap_ptrace_traceme(struct task_struct *parent);
 extern int cap_capget(struct task_struct *target, kernel_cap_t *effective, kernel_cap_t *inheritable, kernel_cap_t *permitted);
 extern int cap_capset(struct cred *new, const struct cred *old,
@@ -1209,7 +1209,7 @@ static inline void security_free_mnt_opt
  *	@alter contains the flag indicating whether changes are to be made.
  *	Return 0 if permission is granted.
  *
- * @ptrace_may_access:
+ * @ptrace_access_check:
  *	Check permission before allowing the current process to trace the
  *	@child process.
  *	Security modules may also want to perform a process tracing check
@@ -1224,7 +1224,7 @@ static inline void security_free_mnt_opt
  *	Check that the @parent process has sufficient permission to trace the
  *	current process before allowing the current process to present itself
  *	to the @parent process for tracing.
- *	The parent process will still have to undergo the ptrace_may_access
+ *	The parent process will still have to undergo the ptrace_access_check
  *	checks before it is allowed to trace this one.
  *	@parent contains the task_struct structure for debugger process.
  *	Return 0 if permission is granted.
@@ -1336,7 +1336,7 @@ static inline void security_free_mnt_opt
 struct security_operations {
 	char name[SECURITY_NAME_MAX + 1];
 
-	int (*ptrace_may_access) (struct task_struct *child, unsigned int mode);
+	int (*ptrace_access_check) (struct task_struct *child, unsigned int mode);
 	int (*ptrace_traceme) (struct task_struct *parent);
 	int (*capget) (struct task_struct *target,
 		       kernel_cap_t *effective,
@@ -1617,7 +1617,7 @@ extern int security_module_enable(struct
 extern int register_security(struct security_operations *ops);
 
 /* Security operations */
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode);
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode);
 int security_ptrace_traceme(struct task_struct *parent);
 int security_capget(struct task_struct *target,
 		    kernel_cap_t *effective,
@@ -1798,10 +1798,10 @@ static inline int security_init(void)
 	return 0;
 }
 
-static inline int security_ptrace_may_access(struct task_struct *child,
+static inline int security_ptrace_access_check(struct task_struct *child,
 					     unsigned int mode)
 {
-	return cap_ptrace_may_access(child, mode);
+	return cap_ptrace_access_check(child, mode);
 }
 
 static inline int security_ptrace_traceme(struct task_struct *parent)
Index: linux/kernel/ptrace.c
===================================================================
--- linux.orig/kernel/ptrace.c
+++ linux/kernel/ptrace.c
@@ -127,7 +127,7 @@ int ptrace_check_attach(struct task_stru
 	return ret;
 }
 
-int __ptrace_may_access(struct task_struct *task, unsigned int mode)
+int __ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	const struct cred *cred = current_cred(), *tcred;
 
@@ -162,14 +162,14 @@ int __ptrace_may_access(struct task_stru
 	if (!dumpable && !capable(CAP_SYS_PTRACE))
 		return -EPERM;
 
-	return security_ptrace_may_access(task, mode);
+	return security_ptrace_access_check(task, mode);
 }
 
-bool ptrace_may_access(struct task_struct *task, unsigned int mode)
+bool ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	int err;
 	task_lock(task);
-	err = __ptrace_may_access(task, mode);
+	err = __ptrace_access_check(task, mode);
 	task_unlock(task);
 	return !err;
 }
@@ -217,7 +217,7 @@ repeat:
 	/* the same process cannot be attached many times */
 	if (task->ptrace & PT_PTRACED)
 		goto bad;
-	retval = __ptrace_may_access(task, PTRACE_MODE_ATTACH);
+	retval = __ptrace_access_check(task, PTRACE_MODE_ATTACH);
 	if (retval)
 		goto bad;
 
Index: linux/security/capability.c
===================================================================
--- linux.orig/security/capability.c
+++ linux/security/capability.c
@@ -863,7 +863,7 @@ struct security_operations default_secur
 
 void security_fixup_ops(struct security_operations *ops)
 {
-	set_to_cap_if_null(ops, ptrace_may_access);
+	set_to_cap_if_null(ops, ptrace_access_check);
 	set_to_cap_if_null(ops, ptrace_traceme);
 	set_to_cap_if_null(ops, capget);
 	set_to_cap_if_null(ops, capset);
Index: linux/security/commoncap.c
===================================================================
--- linux.orig/security/commoncap.c
+++ linux/security/commoncap.c
@@ -79,7 +79,7 @@ int cap_settime(struct timespec *ts, str
 }
 
 /**
- * cap_ptrace_may_access - Determine whether the current process may access
+ * cap_ptrace_access_check - Determine whether the current process may access
  *			   another
  * @child: The process to be accessed
  * @mode: The mode of attachment.
@@ -87,7 +87,7 @@ int cap_settime(struct timespec *ts, str
  * Determine whether a process may access another, returning 0 if permission
  * granted, -ve if denied.
  */
-int cap_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int cap_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
 	int ret = 0;
 
Index: linux/security/root_plug.c
===================================================================
--- linux.orig/security/root_plug.c
+++ linux/security/root_plug.c
@@ -72,7 +72,7 @@ static int rootplug_bprm_check_security 
 
 static struct security_operations rootplug_security_ops = {
 	/* Use the capability functions for some of the hooks */
-	.ptrace_may_access =		cap_ptrace_may_access,
+	.ptrace_access_check =		cap_ptrace_access_check,
 	.ptrace_traceme =		cap_ptrace_traceme,
 	.capget =			cap_capget,
 	.capset =			cap_capset,
Index: linux/security/security.c
===================================================================
--- linux.orig/security/security.c
+++ linux/security/security.c
@@ -127,9 +127,9 @@ int register_security(struct security_op
 
 /* Security operations */
 
-int security_ptrace_may_access(struct task_struct *child, unsigned int mode)
+int security_ptrace_access_check(struct task_struct *child, unsigned int mode)
 {
-	return security_ops->ptrace_may_access(child, mode);
+	return security_ops->ptrace_access_check(child, mode);
 }
 
 int security_ptrace_traceme(struct task_struct *parent)
Index: linux/security/selinux/hooks.c
===================================================================
--- linux.orig/security/selinux/hooks.c
+++ linux/security/selinux/hooks.c
@@ -1854,12 +1854,12 @@ static inline u32 open_file_to_av(struct
 
 /* Hook functions begin here. */
 
-static int selinux_ptrace_may_access(struct task_struct *child,
+static int selinux_ptrace_access_check(struct task_struct *child,
 				     unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(child, mode);
+	rc = cap_ptrace_access_check(child, mode);
 	if (rc)
 		return rc;
 
@@ -5318,7 +5318,7 @@ static int selinux_key_getsecurity(struc
 static struct security_operations selinux_ops = {
 	.name =				"selinux",
 
-	.ptrace_may_access =		selinux_ptrace_may_access,
+	.ptrace_access_check =		selinux_ptrace_access_check,
 	.ptrace_traceme =		selinux_ptrace_traceme,
 	.capget =			selinux_capget,
 	.capset =			selinux_capset,
Index: linux/security/smack/smack_lsm.c
===================================================================
--- linux.orig/security/smack/smack_lsm.c
+++ linux/security/smack/smack_lsm.c
@@ -92,7 +92,7 @@ struct inode_smack *new_inode_smack(char
  */
 
 /**
- * smack_ptrace_may_access - Smack approval on PTRACE_ATTACH
+ * smack_ptrace_access_check - Smack approval on PTRACE_ATTACH
  * @ctp: child task pointer
  * @mode: ptrace attachment mode
  *
@@ -100,11 +100,11 @@ struct inode_smack *new_inode_smack(char
  *
  * Do the capability checks, and require read and write.
  */
-static int smack_ptrace_may_access(struct task_struct *ctp, unsigned int mode)
+static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
 {
 	int rc;
 
-	rc = cap_ptrace_may_access(ctp, mode);
+	rc = cap_ptrace_access_check(ctp, mode);
 	if (rc != 0)
 		return rc;
 
@@ -2826,7 +2826,7 @@ static void smack_release_secctx(char *s
 struct security_operations smack_ops = {
 	.name =				"smack",
 
-	.ptrace_may_access =		smack_ptrace_may_access,
+	.ptrace_access_check =		smack_ptrace_access_check,
 	.ptrace_traceme =		smack_ptrace_traceme,
 	.capget = 			cap_capget,
 	.capset = 			cap_capset,

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Andrew Morton]

On Thu, 7 May 2009 11:49:47 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> The ptrace_may_access() methods are named confusingly - some 
> variants return a bool, while the security subsystem methods have a 
> retval convention.
> 
> Rename it to ptrace_access_check, to reduce the confusion factor. A 
> followup patch eliminates the bool usage.

s/may_access/access_check/ is a poor change.  The new name conveys less
information than the old one.

It's quite clear what the return value from "may_access" means.  It's
less clear what the return value from a function called "access_check"
means.

Switching to something like ptrace_task_accessible() or
ptrace_may_access_task() would be better.

This happens quite often.  The string "check" in the name of a
predicate function is a red flag.

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Thu, 7 May 2009 11:49:47 +0200
> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > The ptrace_may_access() methods are named confusingly - some 
> > variants return a bool, while the security subsystem methods have a 
> > retval convention.
> > 
> > Rename it to ptrace_access_check, to reduce the confusion factor. A 
> > followup patch eliminates the bool usage.
> 
> s/may_access/access_check/ is a poor change.  The new name conveys 
> less information than the old one.
> 
> It's quite clear what the return value from "may_access" means.  

it isnt clear at all. In fact there's two variants: one that returns 
'int' and one that returns 'bool' - the two have inverted values.

> It's less clear what the return value from a function called 
> "access_check" means.
>
> Switching to something like ptrace_task_accessible() or 
> ptrace_may_access_task() would be better.
> 
> This happens quite often.  The string "check" in the name of a 
> predicate function is a red flag.

I disagree. To repeat the argument i made in this thread, the 'may' 
suggests/attracts a logical value, i.e. yes or no, or boolean. But 
that goes against the desire of actual call sites wanting a Linux 
retval.

I.e. any function name that can be plain-English answered with: 
'yes' or 'no' is a red flag for a retval function.

You cannot answer ptrace_access_check() with 'yes' or 'no'. You 
could if it was ptrace_access_ok() or ptrace_may_access.

	Ingo

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Andrew Morton]

On Mon, 11 May 2009 15:39:49 +0200
Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Thu, 7 May 2009 11:49:47 +0200
> > Ingo Molnar <mingo@elte.hu> wrote:
> > 
> > > The ptrace_may_access() methods are named confusingly - some 
> > > variants return a bool, while the security subsystem methods have a 
> > > retval convention.
> > > 
> > > Rename it to ptrace_access_check, to reduce the confusion factor. A 
> > > followup patch eliminates the bool usage.
> > 
> > s/may_access/access_check/ is a poor change.  The new name conveys 
> > less information than the old one.
> > 
> > It's quite clear what the return value from "may_access" means.  
> 
> it isnt clear at all. In fact there's two variants: one that returns 
> 'int' and one that returns 'bool' - the two have inverted values.

Oh.  I was assuming this was a yesno-returning function.

> > It's less clear what the return value from a function called 
> > "access_check" means.
> >
> > Switching to something like ptrace_task_accessible() or 
> > ptrace_may_access_task() would be better.
> > 
> > This happens quite often.  The string "check" in the name of a 
> > predicate function is a red flag.
> 
> I disagree. To repeat the argument i made in this thread, the 'may' 
> suggests/attracts a logical value, i.e. yes or no, or boolean. But 
> that goes against the desire of actual call sites wanting a Linux 
> retval.
> 
> I.e. any function name that can be plain-English answered with: 
> 'yes' or 'no' is a red flag for a retval function.
> 
> You cannot answer ptrace_access_check() with 'yes' or 'no'. You 
> could if it was ptrace_access_ok() or ptrace_may_access.
> 

So what _are_ the semantics of the ptrace_may_access() return value?

<checks the code comments>

<stomps off in a huff>

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Américo Wang]

On Mon, May 11, 2009 at 9:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> I.e. any function name that can be plain-English answered with:
> 'yes' or 'no' is a red flag for a retval function.
>
> You cannot answer ptrace_access_check() with 'yes' or 'no'. You
> could if it was ptrace_access_ok() or ptrace_may_access.


Aha, then why do you agree with this patch? You don't see
ptrace_access_check() returns bool?? :-)

I stand with Andrew, xxx_check() should not be a boolean function,
ptrace_may_access() looks very OK for me...

Re: [patch 1/2] ptrace, security: rename ptrace_may_access => ptrace_access_check

[Ingo Molnar]

* Américo Wang <xiyou.wangcong@gmail.com> wrote:

> On Mon, May 11, 2009 at 9:39 PM, Ingo Molnar <mingo@elte.hu> wrote:
> >
> > I.e. any function name that can be plain-English answered with:
> > 'yes' or 'no' is a red flag for a retval function.
> >
> > You cannot answer ptrace_access_check() with 'yes' or 'no'. You
> > could if it was ptrace_access_ok() or ptrace_may_access.
> 
> Aha, then why do you agree with this patch? You don't see 
> ptrace_access_check() returns bool?? :-)

except that it doesnt ...

	Ingo

[patch 2/2] ptrace: turn ptrace_access_check() into a retval function

[Ingo Molnar]

ptrace_access_check() returns a bool, while most of the ptrace 
access check machinery works with Linux retvals (where 0 indicates 
success, negative indicates an error).

So eliminate the bool and invert the usage at the call sites.

( Note: "< 0" checks are used instead of !0 checks, because that's
  the convention for retval checks and it results in similarly fast
  assembly code. )

[ Impact: cleanup ]

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 fs/proc/array.c        |    2 +-
 fs/proc/base.c         |    8 ++++----
 fs/proc/task_mmu.c     |    2 +-
 include/linux/ptrace.h |    2 +-
 kernel/ptrace.c        |    6 ++++--
 5 files changed, 11 insertions(+), 9 deletions(-)

Index: linux/fs/proc/array.c
===================================================================
--- linux.orig/fs/proc/array.c
+++ linux/fs/proc/array.c
@@ -366,7 +366,7 @@ static int do_task_stat(struct seq_file 
 
 	state = *get_task_state(task);
 	vsize = eip = esp = 0;
-	permitted = ptrace_access_check(task, PTRACE_MODE_READ);
+	permitted = !ptrace_access_check(task, PTRACE_MODE_READ);
 	mm = get_task_mm(task);
 	if (mm) {
 		vsize = task_vsize(mm);
Index: linux/fs/proc/base.c
===================================================================
--- linux.orig/fs/proc/base.c
+++ linux/fs/proc/base.c
@@ -222,7 +222,7 @@ static int check_mem_permission(struct t
 		rcu_read_lock();
 		match = (tracehook_tracer_task(task) == current);
 		rcu_read_unlock();
-		if (match && ptrace_access_check(task, PTRACE_MODE_ATTACH))
+		if (match && !ptrace_access_check(task, PTRACE_MODE_ATTACH))
 			return 0;
 	}
 
@@ -322,7 +322,7 @@ static int proc_pid_wchan(struct task_st
 	wchan = get_wchan(task);
 
 	if (lookup_symbol_name(wchan, symname) < 0)
-		if (!ptrace_access_check(task, PTRACE_MODE_READ))
+		if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 			return 0;
 		else
 			return sprintf(buffer, "%lu", wchan);
@@ -559,7 +559,7 @@ static int proc_fd_access_allowed(struct
 	 */
 	task = get_proc_task(inode);
 	if (task) {
-		allowed = ptrace_access_check(task, PTRACE_MODE_READ);
+		allowed = !ptrace_access_check(task, PTRACE_MODE_READ);
 		put_task_struct(task);
 	}
 	return allowed;
@@ -938,7 +938,7 @@ static ssize_t environ_read(struct file 
 	if (!task)
 		goto out_no_task;
 
-	if (!ptrace_access_check(task, PTRACE_MODE_READ))
+	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out;
 
 	ret = -ENOMEM;
Index: linux/fs/proc/task_mmu.c
===================================================================
--- linux.orig/fs/proc/task_mmu.c
+++ linux/fs/proc/task_mmu.c
@@ -656,7 +656,7 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EACCES;
-	if (!ptrace_access_check(task, PTRACE_MODE_READ))
+	if (ptrace_access_check(task, PTRACE_MODE_READ) < 0)
 		goto out_task;
 
 	ret = -EINVAL;
Index: linux/include/linux/ptrace.h
===================================================================
--- linux.orig/include/linux/ptrace.h
+++ linux/include/linux/ptrace.h
@@ -101,7 +101,7 @@ extern void ptrace_fork(struct task_stru
 /* Returns 0 on success, -errno on denial. */
 extern int __ptrace_access_check(struct task_struct *task, unsigned int mode);
 /* Returns true on success, false on denial. */
-extern bool ptrace_access_check(struct task_struct *task, unsigned int mode);
+extern int ptrace_access_check(struct task_struct *task, unsigned int mode);
 
 static inline int ptrace_reparented(struct task_struct *child)
 {
Index: linux/kernel/ptrace.c
===================================================================
--- linux.orig/kernel/ptrace.c
+++ linux/kernel/ptrace.c
@@ -165,13 +165,15 @@ int __ptrace_access_check(struct task_st
 	return security_ptrace_access_check(task, mode);
 }
 
-bool ptrace_access_check(struct task_struct *task, unsigned int mode)
+int ptrace_access_check(struct task_struct *task, unsigned int mode)
 {
 	int err;
+
 	task_lock(task);
 	err = __ptrace_access_check(task, mode);
 	task_unlock(task);
-	return !err;
+
+	return err;
 }
 
 int ptrace_attach(struct task_struct *task)

Re: [patch 2/2] ptrace: turn ptrace_access_check() into a retval function

[Roland McGrath]

Acked-by: Roland McGrath <roland@redhat.com>

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[Chris Wright]

* Oleg Nesterov (oleg@redhat.com) wrote:
> +	write_lock_irq(&tasklist_lock);
>  	retval = -EPERM;
>  	if (unlikely(task->exit_state))
> -		goto bad;
> +		goto unlock_tasklist;
>  	if (task->ptrace)
> -		goto bad;
> +		goto unlock_tasklist;

So, task->ptrace now protected by tasklist_lock to keep concurrent tracers
from both attaching to same task?  What does this do for setprocattr()?

                task_lock(p);
                tracer = tracehook_tracer_task(p);
                if (tracer)
                        ptsid = task_sid(tracer);
                task_unlock(p);

Looks like it is racy.

cpu1 (tracer)				cpu2 (tracee, changing sid)
-------------				---------------------------
task_lock(tracee);
__ptrace_may_access(tracee, ATTACH);
task_unlock(tracee);
					task_lock(tracee)
<security check passes>			tracer = tracehook_tracer_task(tracee);
					if (tracer) <-- NULL, !tracee->ptrace
					...
					update sid w/out checking against tracer
write_lock_irq(&tasklist_lock);
...
tracee->ptrace = PT_PTRACED;
...
now we are tracing task w/ a sid
that we didn't authorize to trace

What do you think?

thanks,
-chris

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[Oleg Nesterov]

On 05/06, Chris Wright wrote:
>
> * Oleg Nesterov (oleg@redhat.com) wrote:
> > +	write_lock_irq(&tasklist_lock);
> >  	retval = -EPERM;
> >  	if (unlikely(task->exit_state))
> > -		goto bad;
> > +		goto unlock_tasklist;
> >  	if (task->ptrace)
> > -		goto bad;
> > +		goto unlock_tasklist;
>
> So, task->ptrace now protected by tasklist_lock to keep concurrent tracers
> from both attaching to same task?

Yes,

> (re-oredered)
>
> What do you think?

I think I need your help!

> What does this do for setprocattr()?
>
>                 task_lock(p);
>                 tracer = tracehook_tracer_task(p);
>                 if (tracer)
>                         ptsid = task_sid(tracer);
>                 task_unlock(p);
>
> Looks like it is racy.

Looks like you are right, but I don't understand selinux at all.

> cpu1 (tracer)				cpu2 (tracee, changing sid)
> -------------				---------------------------
> task_lock(tracee);
> __ptrace_may_access(tracee, ATTACH);
> task_unlock(tracee);
> 					task_lock(tracee)
> <security check passes>			tracer = tracehook_tracer_task(tracee);
> 					if (tracer) <-- NULL, !tracee->ptrace
> 					...
> 					update sid w/out checking against tracer
> write_lock_irq(&tasklist_lock);
> ...
> tracee->ptrace = PT_PTRACED;
> ...
> now we are tracing task w/ a sid
> that we didn't authorize to trace

But this can happen without this change too?

- cpu2 takes task_lock(), tracehook_tracer_task() returns NULL because
  we are not traced yet.

- cpu1 does ptrace_attach() and succeds, because cpu2 didn't update sid
  yet

- cpu2 continues, it doesn't check avc_has_perm() (tracer == 0) and
  updates sid.

No?

Shouldn't selinux_setprocattr() take ->cred_exec_mutex, like we do in
selinux_bprm_set_creds() path?

Oleg.

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[Chris Wright]

* Oleg Nesterov (oleg@redhat.com) wrote:
> But this can happen without this change too?
> 
> - cpu2 takes task_lock(), tracehook_tracer_task() returns NULL because
>   we are not traced yet.
> 
> - cpu1 does ptrace_attach() and succeds, because cpu2 didn't update sid
>   yet
> 
> - cpu2 continues, it doesn't check avc_has_perm() (tracer == 0) and
>   updates sid.
> 
> No?

Yes.

> Shouldn't selinux_setprocattr() take ->cred_exec_mutex, like we do in
> selinux_bprm_set_creds() path?

I was looking at the same, seems like it to me.  James?

thanks,
-chris
> 
> Oleg.

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[James Morris]

On Wed, 6 May 2009, Chris Wright wrote:

> * Oleg Nesterov (oleg@redhat.com) wrote:
> > But this can happen without this change too?
> > 
> > - cpu2 takes task_lock(), tracehook_tracer_task() returns NULL because
> >   we are not traced yet.
> > 
> > - cpu1 does ptrace_attach() and succeds, because cpu2 didn't update sid
> >   yet
> > 
> > - cpu2 continues, it doesn't check avc_has_perm() (tracer == 0) and
> >   updates sid.
> > 
> > No?
> 
> Yes.
> 
> > Shouldn't selinux_setprocattr() take ->cred_exec_mutex, like we do in
> > selinux_bprm_set_creds() path?
> 
> I was looking at the same, seems like it to me.  James?

As far as I can tell, yes.

(Added David Howells and security folk to the cc -- please make sure at 
least that the LSM list is cc'd when changing code which affects LSM 
modules).


- James
-- 
James Morris
<jmorris@namei.org>

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[Roland McGrath]

> As far as I can tell, yes.
> 
> (Added David Howells and security folk to the cc -- please make sure at 
> least that the LSM list is cc'd when changing code which affects LSM 
> modules).

Good catch, Chris and Oleg!  This one is yet another dhowells blue plate
special, deeply subtle change buried inside the ginormous commit d84f4f9. ;-}
He even mentioned this one in the log:

 (a) selinux_setprocattr() no longer does its check for whether the
	 current ptracer can access processes with the new SID inside the lock
	 that covers getting the ptracer's SID.  Whilst this lock ensures that
	 the check is done with the ptracer pinned, the result is only valid
	 until the lock is released, so there's no point doing it inside the
	 lock.

Before d84f4f9, the extraction, avc check, and SID switch were all under
task_lock().  What David's comment ignores is that "the lock that covers
getting the ptracer's SID" (i.e. task_lock) is also the lock that excludes
ptrace attempts, with their security checks against the (old or new) SID.
i.e.:

			if (!error)
				tsec->sid = sid;
			task_unlock(p);

That ensured that ptrace_attach/ptrace_traceme would be seen to atomically
check the bits that affect the SELinux ptrace controls and change the bits
that affect "if (tracer)".
     
Indeed, cred_exec_mutex is the equivalent lock for that post-d84f4f9.


Thanks,
Roland

Re: [PATCH 3/3] ptrace: do not use task_lock() for attach

[David Howells]

Roland McGrath <roland@redhat.com> wrote:

> Good catch, Chris and Oleg!  This one is yet another dhowells blue plate
> special, deeply subtle change buried inside the ginormous commit d84f4f9. ;-}

Well...  Unfortunately it was an all-or-nothing patch, leastways if you wanted
the kernel to compile afterwards.

> He even mentioned this one in the log:
> 
>  (a) selinux_setprocattr() no longer does its check for whether the
> 	 current ptracer can access processes with the new SID inside the lock
> 	 that covers getting the ptracer's SID.  Whilst this lock ensures that
> 	 the check is done with the ptracer pinned, the result is only valid
> 	 until the lock is released, so there's no point doing it inside the
> 	 lock.

I knew there was a reason I carefully documented the major changes.

> Before d84f4f9, the extraction, avc check, and SID switch were all under
> task_lock().  What David's comment ignores is that "the lock that covers
> getting the ptracer's SID" (i.e. task_lock) is also the lock that excludes
> ptrace attempts, with their security checks against the (old or new) SID.
> i.e.:

I mainly focused on making sure ptrace and execve still worked in relation to
each other.  Unfortunately, I didn't see that selinux_setprocattr() might
interact with ptrace() in the same manner.

> Indeed, cred_exec_mutex is the equivalent lock for that post-d84f4f9.

Yeah.  Perhaps it should be renamed cred_ptrace_mutex.

David