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

[sashiko-bot@kernel.org]

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