Commit
6d25e7b47616cb2db43351210929c8f19dc305a3 ("apparmor: fix refcount leak when
updating the sk_ctx")
fixes a refcount leak by unconditionally getting and putting a reference
to plabel in update_sk_ctx. However, update_sk_ctx can instead inform its
caller (aa_unix_file_perm) whether or not it persists a reference, and
then conditionally put the reference if it is not used. Semantically, this
involves update_sk_ctx now conditionally transferring the refcount, and
informing aa_unix_file_perm whether the refcount was used (and thus
whether it should be put as part of cleanups).
Signed-off-by: Ryan Lee <[email protected]>
---
security/apparmor/af_unix.c | 30 +++++++++++++++++++++---------
1 file changed, 21 insertions(+), 9 deletions(-)
diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index 9ca9d1b890ba..2730f4ad08d3 100644
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -643,13 +643,16 @@ int aa_unix_peer_perm(const struct cred *subj_cred,
peer_label);
}
-/* sk_plabel for comparison only */
-static void update_sk_ctx(struct sock *sk, struct aa_label *label,
+/* sk_plabel for comparison only
+ * Returns whether plabel was assigned to a pointer
+ */
+static bool update_sk_ctx(struct sock *sk, struct aa_label *label,
struct aa_label *plabel)
{
struct aa_label *l, *old;
struct aa_sk_ctx *ctx = aa_sock(sk);
bool update_sk;
+ bool plabel_used = false;
rcu_read_lock();
update_sk = (plabel &&
@@ -658,7 +661,7 @@ static void update_sk_ctx(struct sock *sk, struct aa_label
*label,
!__aa_subj_label_is_cached(label, rcu_dereference(ctx->label));
rcu_read_unlock();
if (!update_sk)
- return;
+ return false;
spin_lock(&unix_sk(sk)->lock);
old = rcu_dereference_protected(ctx->label,
@@ -675,16 +678,17 @@ static void update_sk_ctx(struct sock *sk, struct
aa_label *label,
old = rcu_dereference_protected(ctx->peer,
lockdep_is_held(&unix_sk(sk)->lock));
if (old == plabel) {
- rcu_assign_pointer(ctx->peer_lastupdate,
- aa_get_label(plabel));
+ rcu_assign_pointer(ctx->peer_lastupdate, plabel);
+ plabel_used = true;
} else if (aa_label_is_subset(plabel, old)) {
- rcu_assign_pointer(ctx->peer_lastupdate,
- aa_get_label(plabel));
+ rcu_assign_pointer(ctx->peer_lastupdate, plabel);
rcu_assign_pointer(ctx->peer, aa_get_label(plabel));
aa_put_label(old);
+ plabel_used = true;
} /* else race or a subset - don't update */
}
spin_unlock(&unix_sk(sk)->lock);
+ return plabel_used;
}
static void update_peer_ctx(struct sock *sk, struct aa_sk_ctx *ctx,
@@ -721,6 +725,7 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct
aa_label *label,
struct path path;
bool is_sk_fs;
int error = 0;
+ bool plabel_owner_transfer = false;
AA_BUG(!label);
AA_BUG(!sock);
@@ -799,8 +804,15 @@ int aa_unix_file_perm(const struct cred *subj_cred, struct
aa_label *label,
/* update peer cache to latest successful perm check */
if (error == 0)
- update_sk_ctx(sock->sk, label, plabel);
- aa_put_label(plabel);
+ plabel_owner_transfer = update_sk_ctx(sock->sk, label, plabel);
+
+ /* If plabel ownership was not transferred, plabel can be either null
+ * (we never got a ref) or non-null (we got a ref and nobody else will
+ * use it, so we need to put it). Either way, aa_put_label will do
+ * the right thing.
+ */
+ if (!plabel_owner_transfer)
+ aa_put_label(plabel);
return error;
}
--
2.43.0
