From mboxrd@z Thu Jan 1 00:00:00 1970 From: jannh@google.com (Jann Horn) Date: Wed, 26 Sep 2018 23:18:35 +0200 Subject: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED In-Reply-To: References: <20180926203446.2004-1-casey.schaufler@intel.com> <20180926203446.2004-2-casey.schaufler@intel.com> Message-ID: To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org On Wed, Sep 26, 2018 at 11:16 PM Jann Horn wrote: > > On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler > wrote: > > A ptrace access check with mode PTRACE_MODE_SCHED gets called > > from process switching code. This precludes the use of audit, > > as the locking is incompatible. Don't do audit in the PTRACE_MODE_SCHED > > case. > > Why is this separate from PTRACE_MODE_NOAUDIT? It looks like > apparmor_ptrace_access_check() currently ignores PTRACE_MODE_NOAUDIT. > Could you, instead of adding a new flag, fix the handling of > PTRACE_MODE_NOAUDIT? Er, after looking at more of the series, I see that PTRACE_MODE_SCHED is necessary; but could you handle the "don't audit" part for AppArmor using PTRACE_MODE_NOAUDIT instead? > > Signed-off-by: Casey Schaufler > > --- > > security/apparmor/domain.c | 2 +- > > security/apparmor/include/ipc.h | 2 +- > > security/apparmor/ipc.c | 8 +++++--- > > security/apparmor/lsm.c | 5 +++-- > > 4 files changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c > > index 08c88de0ffda..28300f4c3ef9 100644 > > --- a/security/apparmor/domain.c > > +++ b/security/apparmor/domain.c > > @@ -77,7 +77,7 @@ static int may_change_ptraced_domain(struct aa_label *to_label, > > if (!tracer || unconfined(tracerl)) > > goto out; > > > > - error = aa_may_ptrace(tracerl, to_label, PTRACE_MODE_ATTACH); > > + error = aa_may_ptrace(tracerl, to_label, PTRACE_MODE_ATTACH, true); > > > > out: > > rcu_read_unlock(); > > diff --git a/security/apparmor/include/ipc.h b/security/apparmor/include/ipc.h > > index 5ffc218d1e74..299d1c45fef0 100644 > > --- a/security/apparmor/include/ipc.h > > +++ b/security/apparmor/include/ipc.h > > @@ -34,7 +34,7 @@ struct aa_profile; > > "xcpu xfsz vtalrm prof winch io pwr sys emt lost" > > > > int aa_may_ptrace(struct aa_label *tracer, struct aa_label *tracee, > > - u32 request); > > + u32 request, bool audit); > > int aa_may_signal(struct aa_label *sender, struct aa_label *target, int sig); > > > > #endif /* __AA_IPC_H */ > > diff --git a/security/apparmor/ipc.c b/security/apparmor/ipc.c > > index 527ea1557120..9ed110afc822 100644 > > --- a/security/apparmor/ipc.c > > +++ b/security/apparmor/ipc.c > > @@ -121,15 +121,17 @@ static int profile_tracer_perm(struct aa_profile *tracer, > > * Returns: %0 else error code if permission denied or error > > */ > > int aa_may_ptrace(struct aa_label *tracer, struct aa_label *tracee, > > - u32 request) > > + u32 request, bool audit) > > { > > struct aa_profile *profile; > > u32 xrequest = request << PTRACE_PERM_SHIFT; > > DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_NONE, OP_PTRACE); > > > > return xcheck_labels(tracer, tracee, profile, > > - profile_tracer_perm(profile, tracee, request, &sa), > > - profile_tracee_perm(profile, tracer, xrequest, &sa)); > > + profile_tracer_perm(profile, tracee, request, > > + audit ? &sa : NULL), > > + profile_tracee_perm(profile, tracer, xrequest, > > + audit ? &sa : NULL)); > > } > > > > > > diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c > > index 8b8b70620bbe..da9d0b228857 100644 > > --- a/security/apparmor/lsm.c > > +++ b/security/apparmor/lsm.c > > @@ -118,7 +118,8 @@ static int apparmor_ptrace_access_check(struct task_struct *child, > > tracee = aa_get_task_label(child); > > error = aa_may_ptrace(tracer, tracee, > > (mode & PTRACE_MODE_READ) ? AA_PTRACE_READ > > - : AA_PTRACE_TRACE); > > + : AA_PTRACE_TRACE, > > + !(mode & PTRACE_MODE_SCHED)); > > aa_put_label(tracee); > > end_current_label_crit_section(tracer); > > > > @@ -132,7 +133,7 @@ static int apparmor_ptrace_traceme(struct task_struct *parent) > > > > tracee = begin_current_label_crit_section(); > > tracer = aa_get_task_label(parent); > > - error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE); > > + error = aa_may_ptrace(tracer, tracee, AA_PTRACE_TRACE, true); > > aa_put_label(tracer); > > end_current_label_crit_section(tracee); > > > > -- > > 2.17.1 > >