[PATCH] apparmor: fix race on NULL ctx->peer in update_sk_ctx()

[PATCH] apparmor: fix race on NULL ctx->peer in update_sk_ctx()

[Emanuele Rocca]

The unlocked check in update_sk_ctx() can see peer_lastupdate set before
peer is initialized, passing NULL to aa_label_is_subset().

The earlier NULL checks only cover accesses under the socket lock.
Check for NULL and refresh the cache instead.

Fixes: 88fec3526e84 ("apparmor: make sure unix socket labeling is correctly updated.")
Link: https://lore.kernel.org/all/20260824155822.9214-1-maxime.belair@canonical.com/
Cc: stable@vger.kernel.org
Signed-off-by: Emanuele Rocca <emanuele.rocca@arm.com>
---
 security/apparmor/af_unix.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index d04d9cd268aa..b95959806303 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))) ||
 	  !__aa_subj_label_is_cached(label, rcu_dereference(ctx->label));
 	rcu_read_unlock();
 	if (!update_sk)
-- 
2.55.0

Re: [PATCH] apparmor: fix race on NULL ctx->peer in update_sk_ctx()

[sashiko-bot]

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