* Re: [PATCH] apparmor: fix race on NULL ctx->peer in update_sk_ctx()
2026-10-29 14:06 [PATCH] apparmor: fix race on NULL ctx->peer in update_sk_ctx() Emanuele Rocca
@ 2026-10-29 14:26 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-10-29 14:26 UTC (permalink / raw)
Cc: linux-security-module
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] apparmor: NULL ctx->peer dereference under socket lock in update_sk_ctx()
--
commit 30beb04850046e0aecf0477d58886fc5cc6af632
Author: Emanuele Rocca <emanuele.rocca@arm.com>
apparmor: fix race on NULL ctx->peer in update_sk_ctx()
This commit adds a NULL check on ctx->peer in the unlocked RCU fast path of
update_sk_ctx() before calling aa_label_is_subset(), forcing the slow path
under the socket lock when ctx->peer is NULL.
Link: https://lore.kernel.org/all/20260824155822.9214-1-maxime.belair@canonical.com/
> diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
> index b908e744818c9..c405f08de630a 100644
> --- a/security/apparmor/af_unix.c
> +++ b/security/apparmor/af_unix.c
> @@ -657,9 +657,10 @@ static void update_sk_ctx(struct sock *sk, struct aa_label *label,
> bool update_sk;
>
> rcu_read_lock();
> + old = rcu_dereference(ctx->peer);
> update_sk = (plabel &&
> (plabel != rcu_access_pointer(ctx->peer_lastupdate) ||
> - !aa_label_is_subset(plabel, rcu_dereference(ctx->peer)))) ||
> + !old || !aa_label_is_subset(plabel, old))) ||
[Severity: High]
Does this depend on the prerequisite patch referenced in the Link tag
(https://lore.kernel.org/all/20260824155822.9214-1-maxime.belair@canonical.com/)
that adds the NULL checks on ctx->peer under the socket lock?
In this tree, the commit message notes that "The earlier NULL checks only
cover accesses under the socket lock", but those checks under
unix_sk(sk)->lock are not present in security/apparmor/af_unix.c.
When ctx->peer is NULL (for example, after an AF_UNIX SOCK_DGRAM socket
connects via unix_dgram_connect() -> security_unix_may_send() ->
apparmor_unix_may_send() without setting ctx->peer, followed by
aa_unix_file_perm()), setting update_sk to true when !old causes
update_sk_ctx() to enter the locked section:
security/apparmor/af_unix.c:update_sk_ctx() {
...
if (plabel && rcu_access_pointer(ctx->peer_lastupdate) != plabel) {
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));
} else if (aa_label_is_subset(plabel, old)) {
rcu_assign_pointer(ctx->peer_lastupdate,
aa_get_label(plabel));
rcu_assign_pointer(ctx->peer, aa_get_label(plabel));
aa_put_label(old);
} /* else race or a subset - don't update */
}
...
}
Here, if ctx->peer_lastupdate != plabel and old (ctx->peer) is NULL, could
update_sk_ctx() pass old == NULL as sub into aa_label_is_subset(plabel, old),
triggering AA_BUG(!sub) and a NULL pointer dereference in
__aa_label_next_not_in_set()?
Similarly, if ctx->peer is NULL while ctx->peer_lastupdate == plabel (the
race where peer_lastupdate is visible before peer is set), the unlocked check
now sees !old and sets update_sk = true, but the locked check:
if (plabel && rcu_access_pointer(ctx->peer_lastupdate) != plabel)
evaluates to false and skips initializing ctx->peer.
Also, in aa_unix_file_perm() -> update_peer_ctx(), which runs right before
update_sk_ctx():
security/apparmor/af_unix.c:update_peer_ctx() {
...
spin_lock(&unix_sk(sk)->lock);
old = rcu_dereference_protected(ctx->peer,
lockdep_is_held(&unix_sk(sk)->lock));
l = aa_label_merge(old, label, GFP_ATOMIC);
...
}
is old (ctx->peer) also missing a NULL check before being passed to
aa_label_merge()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261029140643.4092-1-emanuele.rocca@arm.com?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread