lkml.org 
[lkml]   [2013]   [Sep]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] apparmor: remove the "task" arg from may_change_ptraced_domain()
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


\
 
 \ /
  Last update: 2013-09-24 00:01    [W:0.055 / U:2.216 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site