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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
Acked-by: John Johansen <[email protected]>
> ---
> 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;
>
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 <[email protected]>
Acked-by: Richard Guy Briggs <[email protected]>
> ---
> 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 <[email protected]>
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
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 <[email protected]>
Thanks!
Oleg.
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 <[email protected]>
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