[PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Oleg Nesterov]

Unless task == current ptrace_parent(task) is not safe even under
rcu_read_lock() and most of the current users are not right.

So may_change_ptraced_domain(task) looks wrong as well. However it
is always called with task == current so the code is actually fine.
Remove this argument to make this fact clear.

Note: perhaps we should simply kill ptrace_parent(), it buys almost
nothing. And it is obviously racy, perhaps this should be fixed.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 security/apparmor/domain.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 26c607c..8423558 100644
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
 
 /**
  * may_change_ptraced_domain - check if can change profile on ptraced task
- * @task: task we want to change profile of   (NOT NULL)
  * @to_profile: profile to change to  (NOT NULL)
  *
- * Check if the task is ptraced and if so if the tracing task is allowed
+ * Check if current is ptraced and if so if the tracing task is allowed
  * to trace the new domain
  *
  * Returns: %0 or error if change not allowed
  */
-static int may_change_ptraced_domain(struct task_struct *task,
-				     struct aa_profile *to_profile)
+static int may_change_ptraced_domain(struct aa_profile *to_profile)
 {
 	struct task_struct *tracer;
 	struct aa_profile *tracerp = NULL;
 	int error = 0;
 
 	rcu_read_lock();
-	tracer = ptrace_parent(task);
+	tracer = ptrace_parent(current);
 	if (tracer)
 		/* released below */
 		tracerp = aa_get_task_profile(tracer);
@@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
 	}
 
 	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
-		error = may_change_ptraced_domain(current, new_profile);
+		error = may_change_ptraced_domain(new_profile);
 		if (error) {
 			aa_put_profile(new_profile);
 			goto audit;
@@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
 			}
 		}
 
-		error = may_change_ptraced_domain(current, hat);
+		error = may_change_ptraced_domain(hat);
 		if (error) {
 			info = "ptraced";
 			error = -EPERM;
@@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
 	}
 
 	/* check if tracing task is allowed to trace target domain */
-	error = may_change_ptraced_domain(current, target);
+	error = may_change_ptraced_domain(target);
 	if (error) {
 		info = "ptrace prevents transition";
 		goto audit;
-- 
1.5.5.1

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Oleg Nesterov]

On 09/16, Oleg Nesterov wrote:
>
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.

In particular selinux is buggy. But this needs another simple patch,
will do tomorrow.

> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
> 
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  security/apparmor/domain.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>  
>  /**
>   * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of   (NOT NULL)
>   * @to_profile: profile to change to  (NOT NULL)
>   *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
>   * to trace the new domain
>   *
>   * Returns: %0 or error if change not allowed
>   */
> -static int may_change_ptraced_domain(struct task_struct *task,
> -				     struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
>  {
>  	struct task_struct *tracer;
>  	struct aa_profile *tracerp = NULL;
>  	int error = 0;
>  
>  	rcu_read_lock();
> -	tracer = ptrace_parent(task);
> +	tracer = ptrace_parent(current);
>  	if (tracer)
>  		/* released below */
>  		tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	}
>  
>  	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> -		error = may_change_ptraced_domain(current, new_profile);
> +		error = may_change_ptraced_domain(new_profile);
>  		if (error) {
>  			aa_put_profile(new_profile);
>  			goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>  			}
>  		}
>  
> -		error = may_change_ptraced_domain(current, hat);
> +		error = may_change_ptraced_domain(hat);
>  		if (error) {
>  			info = "ptraced";
>  			error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
>  	}
>  
>  	/* check if tracing task is allowed to trace target domain */
> -	error = may_change_ptraced_domain(current, target);
> +	error = may_change_ptraced_domain(target);
>  	if (error) {
>  		info = "ptrace prevents transition";
>  		goto audit;
> -- 
> 1.5.5.1
> 

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[John Johansen]

On 09/16/2013 07:20 AM, Oleg Nesterov wrote:
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.
> 
> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
> 
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: John Johansen <john.johansen@canonical.com>

> ---
>  security/apparmor/domain.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>  
>  /**
>   * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of   (NOT NULL)
>   * @to_profile: profile to change to  (NOT NULL)
>   *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
>   * to trace the new domain
>   *
>   * Returns: %0 or error if change not allowed
>   */
> -static int may_change_ptraced_domain(struct task_struct *task,
> -				     struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
>  {
>  	struct task_struct *tracer;
>  	struct aa_profile *tracerp = NULL;
>  	int error = 0;
>  
>  	rcu_read_lock();
> -	tracer = ptrace_parent(task);
> +	tracer = ptrace_parent(current);
>  	if (tracer)
>  		/* released below */
>  		tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	}
>  
>  	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> -		error = may_change_ptraced_domain(current, new_profile);
> +		error = may_change_ptraced_domain(new_profile);
>  		if (error) {
>  			aa_put_profile(new_profile);
>  			goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>  			}
>  		}
>  
> -		error = may_change_ptraced_domain(current, hat);
> +		error = may_change_ptraced_domain(hat);
>  		if (error) {
>  			info = "ptraced";
>  			error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
>  	}
>  
>  	/* check if tracing task is allowed to trace target domain */
> -	error = may_change_ptraced_domain(current, target);
> +	error = may_change_ptraced_domain(target);
>  	if (error) {
>  		info = "ptrace prevents transition";
>  		goto audit;
> 

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Richard Guy Briggs]

On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> Unless task == current ptrace_parent(task) is not safe even under
> rcu_read_lock() and most of the current users are not right.

Could you point to an explanation of this?

> So may_change_ptraced_domain(task) looks wrong as well. However it
> is always called with task == current so the code is actually fine.
> Remove this argument to make this fact clear.
> 
> Note: perhaps we should simply kill ptrace_parent(), it buys almost
> nothing. And it is obviously racy, perhaps this should be fixed.

(Did you send a patch to fix the selinux hook?)

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Richard Guy Briggs <rgb@redhat.com>

> ---
>  security/apparmor/domain.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
> index 26c607c..8423558 100644
> --- a/security/apparmor/domain.c
> +++ b/security/apparmor/domain.c
> @@ -50,23 +50,21 @@ void aa_free_domain_entries(struct aa_domain *domain)
>  
>  /**
>   * may_change_ptraced_domain - check if can change profile on ptraced task
> - * @task: task we want to change profile of   (NOT NULL)
>   * @to_profile: profile to change to  (NOT NULL)
>   *
> - * Check if the task is ptraced and if so if the tracing task is allowed
> + * Check if current is ptraced and if so if the tracing task is allowed
>   * to trace the new domain
>   *
>   * Returns: %0 or error if change not allowed
>   */
> -static int may_change_ptraced_domain(struct task_struct *task,
> -				     struct aa_profile *to_profile)
> +static int may_change_ptraced_domain(struct aa_profile *to_profile)
>  {
>  	struct task_struct *tracer;
>  	struct aa_profile *tracerp = NULL;
>  	int error = 0;
>  
>  	rcu_read_lock();
> -	tracer = ptrace_parent(task);
> +	tracer = ptrace_parent(current);
>  	if (tracer)
>  		/* released below */
>  		tracerp = aa_get_task_profile(tracer);
> @@ -477,7 +475,7 @@ int apparmor_bprm_set_creds(struct linux_binprm *bprm)
>  	}
>  
>  	if (bprm->unsafe & (LSM_UNSAFE_PTRACE | LSM_UNSAFE_PTRACE_CAP)) {
> -		error = may_change_ptraced_domain(current, new_profile);
> +		error = may_change_ptraced_domain(new_profile);
>  		if (error) {
>  			aa_put_profile(new_profile);
>  			goto audit;
> @@ -690,7 +688,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
>  			}
>  		}
>  
> -		error = may_change_ptraced_domain(current, hat);
> +		error = may_change_ptraced_domain(hat);
>  		if (error) {
>  			info = "ptraced";
>  			error = -EPERM;
> @@ -829,7 +827,7 @@ int aa_change_profile(const char *ns_name, const char *hname, bool onexec,
>  	}
>  
>  	/* check if tracing task is allowed to trace target domain */
> -	error = may_change_ptraced_domain(current, target);
> +	error = may_change_ptraced_domain(target);
>  	if (error) {
>  		info = "ptrace prevents transition";
>  		goto audit;
> -- 
> 1.5.5.1
> 
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Oleg Nesterov]

On 09/23, Richard Guy Briggs wrote:
>
> On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > Unless task == current ptrace_parent(task) is not safe even under
> > rcu_read_lock() and most of the current users are not right.
>
> Could you point to an explanation of this?

If this task exits before rcu_read_lock() ->parent can point to the
already freed/reused memory.

(in the long term we should probably clear
 ->parent/real_parent/group_leader/more in __unhash_process(), but
 lets not discuss this right now ;)

> (Did you send a patch to fix the selinux hook?)

No, sorry, I was sick. Will do.

> Acked-by: Richard Guy Briggs <rgb@redhat.com>

Thanks!

Oleg.

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Richard Guy Briggs]

On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
> On 09/23, Richard Guy Briggs wrote:
> >
> > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > > Unless task == current ptrace_parent(task) is not safe even under
> > > rcu_read_lock() and most of the current users are not right.
> >
> > Could you point to an explanation of this?
> 
> If this task exits before rcu_read_lock() ->parent can point to the
> already freed/reused memory.

Ok, understood.  So even though the task may have exited, the task
struct pointer is still valid, but not the contents of the task struct
to which it points.

> (in the long term we should probably clear
>  ->parent/real_parent/group_leader/more in __unhash_process(), but
>  lets not discuss this right now ;)

...so that the contents are valid in a task struct of a task that has
exited.

Thanks for the (more obvious to me now) explanation.

> Oleg.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer
Kernel Security
AMER ENG Base Operating Systems
Remote, Ottawa, Canada
Voice: +1.647.777.2635
Internal: (81) 32635
Alt: +1.613.693.0684x3545

(no subject)

[Richard Guy Briggs]

Bcc: rgb@redhat.com
Subject: Re: [PATCH] apparmor: remove the "task" arg from
 may_change_ptraced_domain()
Reply-To: 
In-Reply-To: <20130926132519.GY13968@madcap2.tricolour.ca>

On 13/09/26, Richard Guy Briggs wrote:
> On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
> > On 09/23, Richard Guy Briggs wrote:
> > >
> > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > > > Unless task == current ptrace_parent(task) is not safe even under
> > > > rcu_read_lock() and most of the current users are not right.
> > >
> > > Could you point to an explanation of this?
> > 
> > If this task exits before rcu_read_lock() ->parent can point to the
> > already freed/reused memory.
> 
> Ok, understood.  So even though the task may have exited, the task
> struct pointer is still valid, but not the contents of the task struct
> to which it points.

[The thread also relates to the patch
	"pid: get ppid pid_t of task in init_pid_ns safely"
in which sys_getppid() (which appears safe) is replaced with something that
references the init_pid_ns rather than current's pid_ns.]

So, in the general case, that call is not safe, and we should at least
remove the task_struct argument.

Analysing the calling tree, I see that most callers just call with
"current" anyways.  (Read this tree as "is called by" with the provided
"struct task_struct *".)

audit_log_task_info(?)
        audit_log_link_denied(current)
        audit_log_exit(?)
		__audit_free(?)
			audit_free(?)
                		do_exit(current)
        		        copy_process(?)
					[see below]
        ima_audit_measurement(current)
audit_filter_rules(?)
        audit_filter_task(?)
                audit_alloc(?)
			copy_process(?)
				p->real_parent is copy of current->real_parent
        audit_filter_syscall(?)
                audit_get_context(?)
                __audit_syscall_entry(current)
        audit_filter_inode_name(?)
                audit_filter_inodes(?)
                        audit_update_watch(current)
                        audit_get_context(?)
                                __audit_free(?)
                                        audit_free(?)
                                                do_exit(current)
                                                copy_process(?)
							[see below]
                                __audit_syscall_exit(current)

So looking further at copy_process()... 

	copy_flags() looks weird.  The clone_flags parameter is not used.  The
	existing flags (copied from the parent in dup_task_struct()) are
	massaged, removing PF_SUPERPRIV and PF_WQ_WORKER and adding
	PF_FORKNOEXEC.

	copy_process() passes an identical copy of "current" to audit_alloc()
	(as far as task_struct::real_parent is concerned).

	copy_process() passes an identical copy of "current" to audit_free()
	(as far as task_struct::real_parent is concerned) in all cases
	*except* if the original process has a signal_pending() and
	copy_process() was called without CLONE_PARENT or CLONE_THREAD, in
	which case tsk->real_parent is actually current.

	In fact, the actions listed in the true clause of:
		"if (clone_flags & (CLONE_PARENT|CLONE_THREAD))"
	look redundant.

So, can audit_log_task_info() and audit_filter_rules() be fixed to be called
without the task_struct pointer and just use current->real_parent directly?
How do we check for the case where there was a signal pending it and
tsk->real_parent now equals "current"?

> > (in the long term we should probably clear
> >  ->parent/real_parent/group_leader/more in __unhash_process(), but
> >  lets not discuss this right now ;)
> 
> ...so that the contents are valid in a task struct of a task that has
> exited.

The assumptions made above include that sys_getppid() was safe all along.

> Thanks for the (more obvious to me now) explanation.
> 
> > Oleg.
> 
> - RGB

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Oleg Nesterov]

On 12/18, Richard Guy Briggs wrote:
>
> Bcc: rgb@redhat.com
> Subject: Re: [PATCH] apparmor: remove the "task" arg from
>  may_change_ptraced_domain()
> Reply-To:
> In-Reply-To: <20130926132519.GY13968@madcap2.tricolour.ca>

The subject is empty ;) I changed it to match the above.

> On 13/09/26, Richard Guy Briggs wrote:
> > On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
> > > On 09/23, Richard Guy Briggs wrote:
> > > >
> > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > > > > Unless task == current ptrace_parent(task) is not safe even under
> > > > > rcu_read_lock() and most of the current users are not right.
> > > >
> > > > Could you point to an explanation of this?
> > >
> > > If this task exits before rcu_read_lock() ->parent can point to the
> > > already freed/reused memory.
> >
> > Ok, understood.  So even though the task may have exited, the task
> > struct pointer is still valid, but not the contents of the task struct
> > to which it points.
>
> [The thread also relates to the patch
> 	"pid: get ppid pid_t of task in init_pid_ns safely"
> in which sys_getppid() (which appears safe) is replaced with something that
> references the init_pid_ns rather than current's pid_ns.]
>
> So, in the general case, that call is not safe, and we should at least
> remove the task_struct argument.

I changed my mind, please see the recent discussion with Paul:

	http://marc.info/?t=138626281900001

instead we should document why ptrace_parent() is safe without pid_alive().

I hope that the change in apparmor was fine anyway.


Otherwise I can't understand your email, at least right now... I do not
know how/where audit uses parent/real_parent.

But yes, unless tsk == current, the usage of tsk->*parent is not safe even
under rcu_read_lock() unless you verify that this task was not unhashed.

ptrace_parent() is safe because it checks ->ptrace. Previously I thought
we should not rely on this, but the additional pid_alive() looks ugly so
it would be better to simply document this. I'll send the patch.

Oleg.

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Richard Guy Briggs]

On 13/12/18, Oleg Nesterov wrote:
> On 12/18, Richard Guy Briggs wrote:
> >
> > Bcc: rgb@redhat.com
> > Subject: Re: [PATCH] apparmor: remove the "task" arg from
> >  may_change_ptraced_domain()
> > Reply-To:
> > In-Reply-To: <20130926132519.GY13968@madcap2.tricolour.ca>
> 
> The subject is empty ;) I changed it to match the above.

HTH?!?  Thanks for adding it.  (more below...)

> > On 13/09/26, Richard Guy Briggs wrote:
> > > On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
> > > > On 09/23, Richard Guy Briggs wrote:
> > > > >
> > > > > On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
> > > > > > Unless task == current ptrace_parent(task) is not safe even under
> > > > > > rcu_read_lock() and most of the current users are not right.
> > > > >
> > > > > Could you point to an explanation of this?
> > > >
> > > > If this task exits before rcu_read_lock() ->parent can point to the
> > > > already freed/reused memory.
> > >
> > > Ok, understood.  So even though the task may have exited, the task
> > > struct pointer is still valid, but not the contents of the task struct
> > > to which it points.
> >
> > [The thread also relates to the patch
> > 	"pid: get ppid pid_t of task in init_pid_ns safely"
> > in which sys_getppid() (which appears safe) is replaced with something that
> > references the init_pid_ns rather than current's pid_ns.]
> >
> > So, in the general case, that call is not safe, and we should at least
> > remove the task_struct argument.
> 
> I changed my mind, please see the recent discussion with Paul:
> 
> 	http://marc.info/?t=138626281900001
> 
> instead we should document why ptrace_parent() is safe without pid_alive().

Interesting.  I wasn't aware of pid_alive(), but that would certainly
help.

> I hope that the change in apparmor was fine anyway.

Yes, I'm fine with apparmor change, if it was deemed that the ppid
wasn't needed.  If it is, then it should use this new task_ppid_nr().
Better yet I think to generalize it to anticipate auditd in containers.

> Otherwise I can't understand your email, at least right now... I do not
> know how/where audit uses parent/real_parent.

It uses real_parent to include the ppid number of a process in a couple
of log records.

> But yes, unless tsk == current, the usage of tsk->*parent is not safe even
> under rcu_read_lock() unless you verify that this task was not unhashed.

As I said, the only case I can see is in copy_process() when a signal is
pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it.
Still, that is enough to need to check it.

> ptrace_parent() is safe because it checks ->ptrace. Previously I thought
> we should not rely on this, but the additional pid_alive() looks ugly so
> it would be better to simply document this. I'll send the patch.

So what are you saying?  I should use pid_alive() inside the
rcu_read_lock() to verify it is not unhashed since I don't have anything
to do with ->ptrace or any other task lock?  In fact, the answer is
under my nose in __task_pid_nr_ns(), which already uses this approach.
And rcu_read_lock() can be nested.

> Oleg.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[John Johansen]

On 12/19/2013 08:36 PM, Richard Guy Briggs wrote:
> On 13/12/18, Oleg Nesterov wrote:
>> On 12/18, Richard Guy Briggs wrote:
>>>
>>> Bcc: rgb@redhat.com
>>> Subject: Re: [PATCH] apparmor: remove the "task" arg from
>>>  may_change_ptraced_domain()
>>> Reply-To:
>>> In-Reply-To: <20130926132519.GY13968@madcap2.tricolour.ca>
>>
>> The subject is empty ;) I changed it to match the above.
> 
> HTH?!?  Thanks for adding it.  (more below...)
> 
>>> On 13/09/26, Richard Guy Briggs wrote:
>>>> On Tue, Sep 24, 2013 at 06:44:42PM +0200, Oleg Nesterov wrote:
>>>>> On 09/23, Richard Guy Briggs wrote:
>>>>>>
>>>>>> On Mon, Sep 16, 2013 at 04:20:35PM +0200, Oleg Nesterov wrote:
>>>>>>> Unless task == current ptrace_parent(task) is not safe even under
>>>>>>> rcu_read_lock() and most of the current users are not right.
>>>>>>
>>>>>> Could you point to an explanation of this?
>>>>>
>>>>> If this task exits before rcu_read_lock() ->parent can point to the
>>>>> already freed/reused memory.
>>>>
>>>> Ok, understood.  So even though the task may have exited, the task
>>>> struct pointer is still valid, but not the contents of the task struct
>>>> to which it points.
>>>
>>> [The thread also relates to the patch
>>> 	"pid: get ppid pid_t of task in init_pid_ns safely"
>>> in which sys_getppid() (which appears safe) is replaced with something that
>>> references the init_pid_ns rather than current's pid_ns.]
>>>
>>> So, in the general case, that call is not safe, and we should at least
>>> remove the task_struct argument.
>>
>> I changed my mind, please see the recent discussion with Paul:
>>
>> 	http://marc.info/?t=138626281900001
>>
>> instead we should document why ptrace_parent() is safe without pid_alive().
> 
> Interesting.  I wasn't aware of pid_alive(), but that would certainly
> help.
> 
>> I hope that the change in apparmor was fine anyway.
> 
> Yes, I'm fine with apparmor change, if it was deemed that the ppid
> wasn't needed.  If it is, then it should use this new task_ppid_nr().
it wasn't needed, changes where made years ago to allow us to get rid of
using the parent pid. Its was left in for a transition period and just
had never been removed.

> Better yet I think to generalize it to anticipate auditd in containers.
> 
yep, that is the way to go

Re: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()

[Oleg Nesterov]

On 12/19, Richard Guy Briggs wrote:
>
> On 13/12/18, Oleg Nesterov wrote:
>
> > Otherwise I can't understand your email, at least right now... I do not
> > know how/where audit uses parent/real_parent.
>
> It uses real_parent to include the ppid number of a process in a couple
> of log records.

I did a quick grep, it seems that audit uses sys_getppid() which should be
fine. Nevermind, if you meant that (say) audit_alloc() paths use tsk->parent
somehow, I agree this doesn't look right/safe. new_child->*parent can point
to nowhere right after dup_task_struct() and there is no way to detect this.

Unless, of course new_child->*parent == current, but copy_process() changes
child->parent only after it takes tasklist_lock.

> > But yes, unless tsk == current, the usage of tsk->*parent is not safe even
> > under rcu_read_lock() unless you verify that this task was not unhashed.
>
> As I said, the only case I can see is in copy_process() when a signal is
> pending when neither CLONE_PARENT nor CLONE_THREAD is passed to it.
> Still, that is enough to need to check it.

Hmm, so I guess you are worried about audit_free?

But this error path can be called even before it checks signal_pending(),
suppose that copy_semundo() fails?

So it seems that CLONE_PARENT/THREAD doesn't really matter because it
audit_free() can be called before copy_process() sets ->parent = current?

Most probably I misunderstood you, so please ignore. I am sure you fully
understand the problems and do not need my comments ;)

> So what are you saying?  I should use pid_alive() inside the
> rcu_read_lock() to verify it is not unhashed since I don't have anything
> to do with ->ptrace or any other task lock?  In fact, the answer is
> under my nose in __task_pid_nr_ns(), which already uses this approach.

Yes. task->parent (or real_parent) can only make sense if this task can
be re-parented (if parent exits, or debugger detaches). If the task is
dead (removed from the parent->children list), obviously nobody can update
this pointer.

And this reminds me we should simply clear ->parent/real_parent on exit.
And ->group_leader. I'll try to make the patch(s), after I finish
thread_group changes. Unfortunately this needs a lot of grepping.

Oleg.