apparmor: convert to profile block critical sections

There are still a few places where profile replacement fails to update
and a stale profile is used for mediation. Fix this by moving to
accessing the current label through a critical section that will
always ensure mediation is using the current label regardless of
whether the tasks cred has been updated or not.

Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index b64ea21a42adbab76020b8206bd042b0a545ec1c..e2919a0766b00af91aca3aa287db9176a075701f 100644 (file)
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -407,7 +407,10 @@ static ssize_t policy_update(u32 mask, const char __user *buf, size_t size,
 {
        ssize_t error;
        struct aa_loaddata *data;
-       struct aa_profile *profile = aa_current_profile();
+       struct aa_profile *profile;
+
+       profile = begin_current_profile_crit_section();
+
        /* high level check about policy management - fine grained in
         * below after unpack
         */
@@ -421,6 +424,7 @@ static ssize_t policy_update(u32 mask, const char __user *buf, size_t size,
                error = aa_replace_profiles(ns, profile, mask, data);
                aa_put_loaddata(data);
        }
+       end_current_profile_crit_section(profile);
 
        return error;
 }
@@ -468,7 +472,7 @@ static ssize_t profile_remove(struct file *f, const char __user *buf,
        ssize_t error;
        struct aa_ns *ns = aa_get_ns(f->f_inode->i_private);
 
-       profile = aa_current_profile();
+       profile = begin_current_profile_crit_section();
        /* high level check about policy management - fine grained in
         * below after unpack
         */
@@ -489,6 +493,7 @@ static ssize_t profile_remove(struct file *f, const char __user *buf,
                aa_put_loaddata(data);
        }
  out:
+       end_current_profile_crit_section(profile);
        aa_put_ns(ns);
        return error;
 }
@@ -667,8 +672,9 @@ static ssize_t query_data(char *buf, size_t buf_len,
        if (buf_len < sizeof(bytes) + sizeof(blocks))
                return -EINVAL; /* not enough space */
 
-       curr = aa_current_profile();
+       curr = begin_current_profile_crit_section();
        profile = aa_fqlookupn_profile(curr, query, strnlen(query, query_len));
+       end_current_profile_crit_section(curr);
        if (!profile)
                return -ENOENT;
 
@@ -754,8 +760,9 @@ static ssize_t query_label(char *buf, size_t buf_len,
        match_str = label_name + label_name_len + 1;
        match_len = query_len - label_name_len - 1;
 
-       curr = aa_current_profile();
+       curr = begin_current_profile_crit_section();
        profile = aa_fqlookupn_profile(curr, label_name, label_name_len);
+       end_current_profile_crit_section(curr);
        if (!profile)
                return -ENOENT;
 
@@ -1094,18 +1101,22 @@ static const struct file_operations seq_ns_ ##NAME ##_fops = {        \
 
 static int seq_ns_level_show(struct seq_file *seq, void *v)
 {
-       struct aa_ns *ns = aa_current_profile()->ns;
+       struct aa_profile *profile;
 
-       seq_printf(seq, "%d\n", ns->level);
+       profile = begin_current_profile_crit_section();
+       seq_printf(seq, "%d\n", profile->ns->level);
+       end_current_profile_crit_section(profile);
 
        return 0;
 }
 
 static int seq_ns_name_show(struct seq_file *seq, void *v)
 {
-       struct aa_ns *ns = aa_current_profile()->ns;
+       struct aa_profile *profile;
 
-       seq_printf(seq, "%s\n", aa_ns_name(ns, ns, true));
+       profile = begin_current_profile_crit_section();
+       seq_printf(seq, "%s\n", aa_ns_name(profile->ns, profile->ns, true));
+       end_current_profile_crit_section(profile);
 
        return 0;
 }
@@ -1530,9 +1541,9 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
        struct aa_ns *ns, *parent;
        /* TODO: improve permission check */
-       struct aa_profile *profile = aa_current_profile();
+       struct aa_profile *profile = begin_current_profile_crit_section();
        int error = aa_may_manage_policy(profile, NULL, AA_MAY_LOAD_POLICY);
-
+       end_current_profile_crit_section(profile);
        if (error)
                return error;
 
@@ -1576,9 +1587,9 @@ static int ns_rmdir_op(struct inode *dir, struct dentry *dentry)
 {
        struct aa_ns *ns, *parent;
        /* TODO: improve permission check */
-       struct aa_profile *profile = aa_current_profile();
+       struct aa_profile *profile = begin_current_profile_crit_section();
        int error = aa_may_manage_policy(profile, NULL, AA_MAY_LOAD_POLICY);
-
+       end_current_profile_crit_section(profile);
        if (error)
                return error;
 
@@ -1922,10 +1933,9 @@ static struct aa_profile *next_profile(struct aa_ns *root,
 static void *p_start(struct seq_file *f, loff_t *pos)
 {
        struct aa_profile *profile = NULL;
-       struct aa_ns *root = aa_current_profile()->ns;
+       struct aa_ns *root = aa_get_current_ns();
        loff_t l = *pos;
-       f->private = aa_get_ns(root);
-
+       f->private = root;
 
        /* find the first profile */
        mutex_lock(&root->lock);

diff --git a/security/apparmor/context.c b/security/apparmor/context.c
index 1fc16b88efbf7543b424effd19231b08bf7ab824..410b9f7f68a1dfb151e4797d64f25238310b8240 100644 (file)
--- a/security/apparmor/context.c
+++ b/security/apparmor/context.c
@@ -79,7 +79,7 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task)
        struct aa_profile *p;
 
        rcu_read_lock();
-       p = aa_get_profile(__aa_task_profile(task));
+       p = aa_get_newest_profile(__aa_task_raw_profile(task));
        rcu_read_unlock();
 
        return p;

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index 2b1524c79fb8d086953f296cb476c995036d4c36..0c02eac33a45c813c8d26a3c089b611f12cfbdca 100644 (file)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -594,7 +594,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, bool permtest)
        /* released below */
        cred = get_current_cred();
        ctx = cred_ctx(cred);
-       profile = aa_get_newest_profile(aa_cred_profile(cred));
+       profile = aa_get_newest_cred_profile(cred);
        previous_profile = aa_get_newest_profile(ctx->previous);
 
        if (unconfined(profile)) {
@@ -737,7 +737,7 @@ int aa_change_profile(const char *fqname, bool onexec,
        }
 
        cred = get_current_cred();
-       profile = aa_cred_profile(cred);
+       profile = aa_get_newest_cred_profile(cred);
 
        /*
         * Fail explicitly requested domain transitions if no_new_privs
@@ -795,6 +795,7 @@ audit:
                                      fqname, GLOBAL_ROOT_UID, info, error);
 
        aa_put_profile(target);
+       aa_put_profile(profile);
        put_cred(cred);
 
        return error;

diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
index 420cfd04121850d5f15e505c0e502815f3228e21..7665fae7131f238fe398db2156b41c9f6540ae64 100644 (file)
--- a/security/apparmor/include/context.h
+++ b/security/apparmor/include/context.h
@@ -56,14 +56,14 @@ struct aa_profile *aa_get_task_profile(struct task_struct *task);
 
 
 /**
- * aa_cred_profile - obtain cred's profiles
+ * aa_cred_raw_profile - obtain cred's profiles
  * @cred: cred to obtain profiles from  (NOT NULL)
  *
  * Returns: confining profile
  *
  * does NOT increment reference count
  */
-static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
+static inline struct aa_profile *aa_cred_raw_profile(const struct cred *cred)
 {
        struct aa_task_ctx *ctx = cred_ctx(cred);
 
@@ -72,16 +72,28 @@ static inline struct aa_profile *aa_cred_profile(const struct cred *cred)
 }
 
 /**
- * __aa_task_profile - retrieve another task's profile
+ * aa_get_newest_cred_profile - obtain the newest profile on a cred
+ * @cred: cred to obtain profile from (NOT NULL)
+ *
+ * Returns: newest version of confining profile
+ */
+static inline
+struct aa_profile *aa_get_newest_cred_profile(const struct cred *cred)
+{
+       return aa_get_newest_profile(aa_cred_raw_profile(cred));
+}
+
+/**
+ * __aa_task_raw_profile - retrieve another task's profile
  * @task: task to query  (NOT NULL)
  *
  * Returns: @task's profile without incrementing its ref count
  *
  * If @task != current needs to be called in RCU safe critical section
  */
-static inline struct aa_profile *__aa_task_profile(struct task_struct *task)
+static inline struct aa_profile *__aa_task_raw_profile(struct task_struct *task)
 {
-       return aa_cred_profile(__task_cred(task));
+       return aa_cred_raw_profile(__task_cred(task));
 }
 
 /**
@@ -92,50 +104,115 @@ static inline struct aa_profile *__aa_task_profile(struct task_struct *task)
  */
 static inline bool __aa_task_is_confined(struct task_struct *task)
 {
-       return !unconfined(__aa_task_profile(task));
+       return !unconfined(__aa_task_raw_profile(task));
 }
 
 /**
- * __aa_current_profile - find the current tasks confining profile
+ * aa_current_raw_profile - find the current tasks confining profile
  *
  * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
  *
  * This fn will not update the tasks cred to the most up to date version
  * of the profile so it is safe to call when inside of locks.
  */
-static inline struct aa_profile *__aa_current_profile(void)
+static inline struct aa_profile *aa_current_raw_profile(void)
 {
-       return aa_cred_profile(current_cred());
+       return aa_cred_raw_profile(current_cred());
 }
 
 /**
- * aa_current_profile - find the current tasks confining profile and do updates
+ * aa_get_current_profile - get the newest version of the current tasks profile
  *
- * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
+ * Returns: newest version of confining profile (NOT NULL)
+ *
+ * This fn will not update the tasks cred, so it is safe inside of locks
  *
- * This fn will update the tasks cred structure if the profile has been
- * replaced.  Not safe to call inside locks
+ * The returned reference must be put with aa_put_profile()
  */
-static inline struct aa_profile *aa_current_profile(void)
+static inline struct aa_profile *aa_get_current_profile(void)
 {
-       const struct aa_task_ctx *ctx = current_ctx();
-       struct aa_profile *profile;
+       struct aa_profile *p = aa_current_raw_profile();
 
-       AA_BUG(!ctx || !ctx->profile);
+       if (profile_is_stale(p))
+               return aa_get_newest_profile(p);
+       return aa_get_profile(p);
+}
 
-       if (profile_is_stale(ctx->profile)) {
-               profile = aa_get_newest_profile(ctx->profile);
-               aa_replace_current_profile(profile);
+#define __end_current_profile_crit_section(X) \
+       end_current_profile_crit_section(X)
+
+/**
+ * end_profile_crit_section - put a reference found with begin_current_profile..
+ * @profile: profile reference to put
+ *
+ * Should only be used with a reference obtained with
+ * begin_current_profile_crit_section and never used in situations where the
+ * task cred may be updated
+ */
+static inline void end_current_profile_crit_section(struct aa_profile *profile)
+{
+       if (profile != aa_current_raw_profile())
                aa_put_profile(profile);
-               ctx = current_ctx();
+}
+
+/**
+ * __begin_current_profile_crit_section - current's confining profile
+ *
+ * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
+ *
+ * safe to call inside locks
+ *
+ * The returned reference must be put with __end_current_profile_crit_section()
+ * This must NOT be used if the task cred could be updated within the
+ * critical section between __begin_current_profile_crit_section() ..
+ * __end_current_profile_crit_section()
+ */
+static inline struct aa_profile *__begin_current_profile_crit_section(void)
+{
+       struct aa_profile *profile = aa_current_raw_profile();
+
+       if (profile_is_stale(profile))
+               profile = aa_get_newest_profile(profile);
+
+       return profile;
+}
+
+/**
+ * begin_current_profile_crit_section - current's profile and update if needed
+ *
+ * Returns: up to date confining profile or the ns unconfined profile (NOT NULL)
+ *
+ * Not safe to call inside locks
+ *
+ * The returned reference must be put with end_current_profile_crit_section()
+ * This must NOT be used if the task cred could be updated within the
+ * critical section between begin_current_profile_crit_section() ..
+ * end_current_profile_crit_section()
+ */
+static inline struct aa_profile *begin_current_profile_crit_section(void)
+{
+       struct aa_profile *profile = aa_current_raw_profile();
+
+       if (profile_is_stale(profile)) {
+               profile = aa_get_newest_profile(profile);
+               if (aa_replace_current_profile(profile) == 0)
+                       /* task cred will keep the reference */
+                       aa_put_profile(profile);
        }
 
-       return ctx->profile;
+       return profile;
 }
 
 static inline struct aa_ns *aa_get_current_ns(void)
 {
-       return aa_get_ns(__aa_current_profile()->ns);
+       struct aa_profile *profile;
+       struct aa_ns *ns;
+
+       profile  = __begin_current_profile_crit_section();
+       ns = aa_get_ns(profile->ns);
+       __end_current_profile_crit_section(profile);
+
+       return ns;
 }
 
 /**

diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 35492008658fe141a630d964ce370ad5d511a9aa..49b780b4c53bb64d062b4621c4ce22733f993341 100644 (file)
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -120,7 +120,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
 
        rcu_read_lock();
        cred = __task_cred(target);
-       profile = aa_cred_profile(cred);
+       profile = aa_get_newest_cred_profile(cred);
 
        /*
         * cap_capget is stacked ahead of this and will
@@ -131,6 +131,7 @@ static int apparmor_capget(struct task_struct *target, kernel_cap_t *effective,
                *permitted = cap_intersect(*permitted, profile->caps.allow);
        }
        rcu_read_unlock();
+       aa_put_profile(profile);
 
        return 0;
 }
@@ -141,9 +142,11 @@ static int apparmor_capable(const struct cred *cred, struct user_namespace *ns,
        struct aa_profile *profile;
        int error = 0;
 
-       profile = aa_cred_profile(cred);
+       profile = aa_get_newest_cred_profile(cred);
        if (!unconfined(profile))
                error = aa_capable(profile, cap, audit);
+       aa_put_profile(profile);
+
        return error;
 }
 
@@ -162,9 +165,10 @@ static int common_perm(const char *op, const struct path *path, u32 mask,
        struct aa_profile *profile;
        int error = 0;
 
-       profile = __aa_current_profile();
+       profile = __begin_current_profile_crit_section();
        if (!unconfined(profile))
                error = aa_path_perm(op, profile, path, 0, mask, cond);
+       __end_current_profile_crit_section(profile);
 
        return error;
 }
@@ -297,9 +301,11 @@ static int apparmor_path_link(struct dentry *old_dentry, const struct path *new_
        if (!path_mediated_fs(old_dentry))
                return 0;
 
-       profile = aa_current_profile();
+       profile = begin_current_profile_crit_section();
        if (!unconfined(profile))
                error = aa_path_link(profile, old_dentry, new_dir, new_dentry);
+       end_current_profile_crit_section(profile);
+
        return error;
 }
 
@@ -312,7 +318,7 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d
        if (!path_mediated_fs(old_dentry))
                return 0;
 
-       profile = aa_current_profile();
+       profile = begin_current_profile_crit_section();
        if (!unconfined(profile)) {
                struct path old_path = { .mnt = old_dir->mnt,
                                         .dentry = old_dentry };
@@ -332,6 +338,8 @@ static int apparmor_path_rename(const struct path *old_dir, struct dentry *old_d
                                             AA_MAY_CREATE, &cond);
 
        }
+       end_current_profile_crit_section(profile);
+
        return error;
 }
 
@@ -369,7 +377,7 @@ static int apparmor_file_open(struct file *file, const struct cred *cred)
                return 0;
        }
 
-       profile = aa_cred_profile(cred);
+       profile = aa_get_newest_cred_profile(cred);
        if (!unconfined(profile)) {
                struct inode *inode = file_inode(file);
                struct path_cond cond = { inode->i_uid, inode->i_mode };
@@ -379,18 +387,23 @@ static int apparmor_file_open(struct file *file, const struct cred *cred)
                /* todo cache full allowed permissions set and state */
                fctx->allow = aa_map_file_to_perms(file);
        }
+       aa_put_profile(profile);
 
        return error;
 }
 
 static int apparmor_file_alloc_security(struct file *file)
 {
+       int error = 0;
+
        /* freed by apparmor_file_free_security */
+       struct aa_profile *profile = begin_current_profile_crit_section();
        file->f_security = aa_alloc_file_context(GFP_KERNEL);
        if (!file->f_security)
                return -ENOMEM;
-       return 0;
+       end_current_profile_crit_section(profile);
 
+       return error;
 }
 
 static void apparmor_file_free_security(struct file *file)
@@ -403,16 +416,17 @@ static void apparmor_file_free_security(struct file *file)
 static int common_file_perm(const char *op, struct file *file, u32 mask)
 {
        struct aa_file_ctx *fctx = file->f_security;
-       struct aa_profile *profile, *fprofile = aa_cred_profile(file->f_cred);
+       struct aa_profile *profile, *fprofile;
        int error = 0;
 
+       fprofile = aa_cred_raw_profile(file->f_cred);
        AA_BUG(!fprofile);
 
        if (!file->f_path.mnt ||
            !path_mediated_fs(file->f_path.dentry))
                return 0;
 
-       profile = __aa_current_profile();
+       profile = __begin_current_profile_crit_section();
 
        /* revalidate access, if task is unconfined, or the cached cred
         * doesn't match or if the request is for more permissions than
@@ -424,6 +438,7 @@ static int common_file_perm(const char *op, struct file *file, u32 mask)
        if (!unconfined(profile) && !unconfined(fprofile) &&
            ((fprofile != profile) || (mask & ~fctx->allow)))
                error = aa_file_perm(op, profile, file, mask);
+       __end_current_profile_crit_section(profile);
 
        return error;
 }
@@ -568,10 +583,11 @@ out:
        return error;
 
 fail:
-       aad(&sa)->profile = aa_current_profile();
+       aad(&sa)->profile = begin_current_profile_crit_section();
        aad(&sa)->info = name;
        aad(&sa)->error = error = -EINVAL;
        aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL);
+       end_current_profile_crit_section(aad(&sa)->profile);
        goto out;
 }
 
@@ -581,7 +597,7 @@ fail:
  */
 static void apparmor_bprm_committing_creds(struct linux_binprm *bprm)
 {
-       struct aa_profile *profile = __aa_current_profile();
+       struct aa_profile *profile = aa_current_raw_profile();
        struct aa_task_ctx *new_ctx = cred_ctx(bprm->cred);
 
        /* bail out if unconfined or not changing profile */
@@ -608,11 +624,12 @@ static void apparmor_bprm_committed_creds(struct linux_binprm *bprm)
 static int apparmor_task_setrlimit(struct task_struct *task,
                unsigned int resource, struct rlimit *new_rlim)
 {
-       struct aa_profile *profile = __aa_current_profile();
+       struct aa_profile *profile = __begin_current_profile_crit_section();
        int error = 0;
 
        if (!unconfined(profile))
                error = aa_task_setrlimit(profile, task, resource, new_rlim);
+       __end_current_profile_crit_section(profile);
 
        return error;
 }

diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c
index e521df1bd1fbe50aa39bd4c22267f8674ac31e01..cac69f2cb86d3a37c729e9a6302a4b2df3125abd 100644 (file)
--- a/security/apparmor/policy_unpack.c
+++ b/security/apparmor/policy_unpack.c
@@ -107,7 +107,7 @@ static int audit_iface(struct aa_profile *new, const char *ns_name,
                       const char *name, const char *info, struct aa_ext *e,
                       int error)
 {
-       struct aa_profile *profile = __aa_current_profile();
+       struct aa_profile *profile = aa_current_raw_profile();
        DEFINE_AUDIT_DATA(sa, LSM_AUDIT_DATA_NONE, NULL);
        if (e)
                aad(&sa)->iface.pos = e->pos - e->start;

diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c
index 3466a27bca098fc4d75324fc5df4f744d43e8077..41b7b64a906b0fbf9d36fe0482ab668059681556 100644 (file)
--- a/security/apparmor/procattr.c
+++ b/security/apparmor/procattr.c
@@ -41,7 +41,7 @@ int aa_getprocattr(struct aa_profile *profile, char **string)
        const char *mode_str = aa_profile_mode_names[profile->mode];
        const char *ns_name = NULL;
        struct aa_ns *ns = profile->ns;
-       struct aa_ns *current_ns = __aa_current_profile()->ns;
+       struct aa_ns *current_ns = aa_get_current_ns();
        char *s;
 
        if (!aa_ns_visible(current_ns, ns, true))
@@ -75,6 +75,7 @@ int aa_getprocattr(struct aa_profile *profile, char **string)
        else
                sprintf(s, "%s (%s)\n", profile->base.hname, mode_str);
        *string = str;
+       aa_put_ns(current_ns);
 
        /* NOTE: len does not include \0 of string, not saved as part of file */
        return len;

diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c
index ae66151fdc38758592c9638dfdf0fe35aa99da9d..b26f1dac5106a5f9f03fbd5d0e049013943b62c1 100644 (file)
--- a/security/apparmor/resource.c
+++ b/security/apparmor/resource.c
@@ -90,7 +90,7 @@ int aa_task_setrlimit(struct aa_profile *profile, struct task_struct *task,
        int error = 0;
 
        rcu_read_lock();
-       task_profile = aa_get_profile(aa_cred_profile(__task_cred(task)));
+       task_profile = aa_get_newest_cred_profile((__task_cred(task)));
        rcu_read_unlock();
 
        /* TODO: extend resource control to handle other (non current)