Commit [Zygmunt's patch posted to https://lists.ubuntu.com/archives/apparmor/2026-May/014620.html]
uncovered a latent refcount issue in that plabel was unconditionally put at the end of aa_unix_file_perm, even if update_sk_ctx transferred ownership via rcu_assign_pointer. This would result in plabel being prematurely freed while still in use. (Without the patch, the plabel seen by update_sk_ctx and the aa_put_label call would always be null, turning those calls into no-ops.) To correctly avoid freeing plabel prematurely, change update_sk_ctx to return whether or not it transferred ownership and condition the aa_put_label call on this boolean. Signed-off-by: Ryan Lee <[email protected]> --- This patch was tested against the Ubuntu 6.17 kernel. The latent issue was exposed by Zygmunt's patch, but it the patch should apply cleanly regardless of whether Zygmunt's patch was applied or not. security/apparmor/af_unix.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c index 3dce3c9c360e..8574ed59251e 100644 --- a/security/apparmor/af_unix.c +++ b/security/apparmor/af_unix.c @@ -642,13 +642,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 && @@ -657,7 +660,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,13 +678,16 @@ static void update_sk_ctx(struct sock *sk, struct aa_label *label, if (old == 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, 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, @@ -718,6 +724,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); @@ -790,8 +797,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
