-----Original Message-----
From: Jann Horn [mailto:jannh at google.com]
Sent: Wednesday, September 26, 2018 2:19 PM
To: Schaufler, Casey <redacted>
Cc: Kernel Hardening <redacted>; kernel list
[off-list ref]; linux-security-module <linux-security-
module at vger.kernel.org>; selinux at tycho.nsa.gov; Hansen, Dave
[off-list ref]; Dock, Deneen T [off-list ref];
kristen at linux.intel.com; Arjan van de Ven [off-list ref]
Subject: Re: [PATCH v5 1/5] AppArmor: Prepare for PTRACE_MODE_SCHED

On Wed, Sep 26, 2018 at 11:16 PM Jann Horn [off-list ref] wrote:

quoted

On Wed, Sep 26, 2018 at 10:35 PM Casey Schaufler
[off-list ref] wrote:

quoted

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.

quoted

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?

I could have done it a number of ways, but this seemed to maintain
the apparmor AA_PTRACE abstraction the best. If aa_may_ptrace didn't
eschew PTRACE_MODE in favor of AA_PTRACE no change to the interface
would have been required. I'm reluctant to change something like that.

quoted

quoted

Signed-off-by: Casey Schaufler <redacted>
---
 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,

quoted

quoted

        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);

quoted

quoted

 out:
        rcu_read_unlock();

diff --git a/security/apparmor/include/ipc.h

b/security/apparmor/include/ipc.h

quoted

quoted

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,

quoted

quoted

  * 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,

quoted

quoted

        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)

quoted

quoted

        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