Hey,
I don't think this is correct. At least I don't see the leakage.
On Mon, 2026-07-06 at 14:36 -0700, Ryan Lee via AppArmor wrote:
> 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);
In apparmor_sk_free_security we put the label on both peer and
peer_lastupdate, so this would cause 2 puts for 1 get, which isn't
right.
> 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);
I don't think the put above was meant to address the "get" done in
update_sk_ctx, I believe it was meant for the plabel =
aa_get_newest_label(pctx->label);
And the gets done in update_sk_ctx would be addressed by
apparmor_sk_free_security.
> + 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;
> }
