apparmor: change fn_label_build() call to not return NULL

Previously fn_label_build() was accepting a NULL which represented
ENOMEM return and ERR_PTR for errors.

Clean this up by requiring the cb fn to return an ERR_PTR or valid
value.

Reviewed-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>

diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c
index c2652165a5886d5f2aa85b0023c2a5c49db42226..4172dedaba2227a9a99574b8b1b8cc46fab009c5 100644 (file)
--- a/security/apparmor/domain.c
+++ b/security/apparmor/domain.c
@@ -864,6 +864,15 @@ audit:
 }
 
 /* ensure none ns domain transitions are correctly applied with onexec */
+static struct aa_label *label_merge_wrap(struct aa_label *a, struct aa_label *b,
+                                        gfp_t gfp)
+{
+       struct aa_label *label = aa_label_merge(a, b, gfp);
+
+       if (!label)
+               return ERR_PTR(-ENOMEM);
+       return label;
+}
 
 static struct aa_label *handle_onexec(const struct cred *subj_cred,
                                      struct aa_label *label,
@@ -891,12 +900,13 @@ static struct aa_label *handle_onexec(const struct cred *subj_cred,
                return ERR_PTR(error);
 
        new = fn_label_build_in_scope(label, profile, GFP_KERNEL,
-                       stack ? aa_label_merge(&profile->label, onexec,
-                                              GFP_KERNEL)
+                       stack ? label_merge_wrap(&profile->label, onexec,
+                                                GFP_KERNEL)
                              : aa_get_newest_label(onexec),
                        profile_transition(subj_cred, profile, bprm,
                                           buffer, cond, unsafe));
-       if (new)
+       AA_BUG(!new);
+       if (!IS_ERR(new))
                return new;
 
        /* TODO: get rid of GLOBAL_ROOT_UID */
@@ -905,7 +915,8 @@ static struct aa_label *handle_onexec(const struct cred *subj_cred,
                                      OP_CHANGE_ONEXEC,
                                      AA_MAY_ONEXEC, bprm->filename, NULL,
                                      onexec, GLOBAL_ROOT_UID,
-                                     "failed to build target label", -ENOMEM));
+                                     "failed to build target label",
+                                     PTR_ERR(new)));
        return ERR_PTR(error);
 }
 
@@ -968,14 +979,10 @@ int apparmor_bprm_creds_for_exec(struct linux_binprm *bprm)
                                profile_transition(subj_cred, profile, bprm,
                                                   buffer,
                                                   &cond, &unsafe));
-
        AA_BUG(!new);
        if (IS_ERR(new)) {
                error = PTR_ERR(new);
                goto done;
-       } else if (!new) {
-               error = -ENOMEM;
-               goto done;
        }
 
        /* Policy has specified a domain transitions. If no_new_privs and
@@ -1223,12 +1230,10 @@ build:
                                   build_change_hat(subj_cred, profile, name,
                                                    sibling),
                                   aa_get_label(&profile->label));
-       if (!new) {
-               info = "label build failed";
-               error = -ENOMEM;
-               goto fail;
-       } /* else if (IS_ERR) build_change_hat has logged error so return new */
        mutex_unlock(&ns->lock);
+       AA_BUG(!new);
+       /* return new label or error ptr */
+
        return new;
 }
 
@@ -1556,7 +1561,8 @@ check:
                new = fn_label_build_in_scope(label, profile, GFP_KERNEL,
                                           aa_get_label(target),
                                           aa_get_label(&profile->label));
-               if (IS_ERR_OR_NULL(new))
+               AA_BUG(!new);
+               if (IS_ERR(new))
                        goto build_fail;
                /*
                 * no new privs prevents domain transitions that would
@@ -1580,10 +1586,9 @@ check:
                        goto build_fail;
                error = aa_replace_current_label(new);
        } else {
-               if (new) {
-                       aa_put_label(new);
-                       new = NULL;
-               }
+               /* new will be recomputed so at exec time. So discard */
+               aa_put_label(new);
+               new = NULL;
 
                /* full transition will be built in exec path */
                aa_set_current_onexec(target, stack);

diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h
index 8c6ce84845526f167757c5e9b397029a72304896..a093304fc998acfd2a2566b72691a597a29d697d 100644 (file)
--- a/security/apparmor/include/lib.h
+++ b/security/apparmor/include/lib.h
@@ -282,7 +282,6 @@ void aa_policy_destroy(struct aa_policy *policy);
  *
  * Returns: new label on success
  *          ERR_PTR if build @FN fails
- *          NULL if label_build fails due to low memory conditions
  *
  * @FN must return a label or ERR_PTR on failure. NULL is not allowed
  */
@@ -298,7 +297,7 @@ void aa_policy_destroy(struct aa_policy *policy);
                DEFINE_VEC(label, __lvec);                              \
                DEFINE_VEC(profile, __pvec);                            \
                if (vec_setup(label, __lvec, (L)->size, (GFP))) {       \
-                       __new_ = NULL;                                  \
+                       __new_ = ERR_PTR(-ENOMEM);                      \
                        goto __done;                                    \
                }                                                       \
                __j = 0;                                                \
@@ -320,23 +319,24 @@ void aa_policy_destroy(struct aa_policy *policy);
                        if (__count > 1) {                              \
                                __new_ = aa_vec_find_or_create_label(__pvec,\
                                                     __count, (GFP));   \
-                               /* only fails if out of Mem */          \
                                if (!__new_)                            \
-                                       __new_ = NULL;                  \
+                                       __new_ = ERR_PTR(-ENOMEM);      \
                        } else                                          \
                                __new_ = aa_get_label(&__pvec[0]->label); \
                        vec_cleanup(profile, __pvec, __count);          \
                } else                                                  \
-                       __new_ = NULL;                                  \
+                       __new_ = ERR_PTR(-ENOMEM);                      \
 __do_cleanup:                                                          \
                vec_cleanup(label, __lvec, (L)->size);                  \
        } else {                                                        \
                (P) = labels_profile(L);                                \
                __new_ = (FN);                                          \
+               AA_BUG(!__new_);                                        \
        }                                                               \
 __done:                                                                        \
-       if (!__new_)                                                    \
+       if (PTR_ERR(__new_))                                            \
                AA_DEBUG(DEBUG_LABEL, "label build failed\n");          \
+       AA_BUG(!__new_);                                                \
        (__new_);                                                       \
 })
 

diff --git a/security/apparmor/mount.c b/security/apparmor/mount.c
index 523570aa1a5a1be213e144482ac024ffd5c9c63e..2f5d918832c10c100ce8a1bb0b74d873d9fd7259 100644 (file)
--- a/security/apparmor/mount.c
+++ b/security/apparmor/mount.c
@@ -735,17 +735,11 @@ int aa_pivotroot(const struct cred *subj_cred, struct aa_label *label,
                        build_pivotroot(subj_cred, profile, new_path,
                                        new_buffer,
                                        old_path, old_buffer));
-       if (!target) {
-               info = "label build failed";
-               error = -ENOMEM;
-               goto fail;
-       } else if (!IS_ERR(target)) {
+       AA_BUG(!target);
+       if (!IS_ERR(target)) {
                error = aa_replace_current_label(target);
-               if (error) {
-                       /* TODO: audit target */
-                       aa_put_label(target);
-                       goto out;
-               }
+               if (error)
+                       goto fail;
                aa_put_label(target);
        } else
                /* already audited error */
@@ -763,7 +757,8 @@ fail:
                                    NULL /*new_name */,
                                    NULL /* old_name */,
                                    NULL, NULL,
-                                   0, NULL, AA_MAY_PIVOTROOT, &nullperms, info,
+                                   0, target->hname, AA_MAY_PIVOTROOT, &nullperms, info,
                                    error));
+       aa_put_label(target);
        goto out;
 }