All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: "Georgia Garcia" <georgia.garcia@canonical.com>,
	"John Johansen" <john.johansen@canonical.com>,
	"Maxime Bélair" <maxime.belair@canonical.com>
Cc: apparmor@lists.ubuntu.com, Aurelien Jarno <aurelien@aurel32.net>,
	linux-security-module@vger.kernel.org
Subject: Re: [apparmor] [PATCH] apparmor: fix NULL ctx->peer derefs in unix socket ctx updates
Date: Wed, 30 Sep 2026 12:19:52 +0200	[thread overview]
Message-ID: <1790762923.ps091h0nv7.astroid@yuna.none> (raw)
In-Reply-To: <20260824155822.9214-1-maxime.belair@canonical.com>

On August 24, 2026 5:58 pm, Maxime Bélair via AppArmor wrote:
> aa_unix_file_perm lazily refreshes the AppArmor context cached on a unix
> socket. Two of the helpers it uses assume ctx->peer has already been set:
> 
> 	update_peer_ctx ->  l = aa_label_merge(old, label, GFP_ATOMIC);
> 	update_sk_ctx   ->  } else if (aa_label_is_subset(plabel, old)) {
> 
> where @old is ctx->peer. Neither aa_label_merge nor aa_label_is_subset
> allows NULL. So both fault on the aa_label->size load.
> 
>   BUG: kernel NULL pointer dereference, address: 000000000000004c
>   RIP: 0010:__aa_label_next_not_in_set+0xb/0xd0
>   Call Trace:
>    aa_label_is_subset+0x3f/0x70
>    aa_unix_file_perm+0x5e8/0x9d0
>    aa_file_perm+0x45a/0x550
>    apparmor_file_permission+0x44/0xb0
>    security_file_permission+0x40/0x100
>    rw_verify_area+0x56/0x180
>    vfs_write+0x7c/0x480
>    ksys_write+0xbf/0xf0
> 
> ctx->peer is only recorded for stream connections and socket pairs.
> unix_dgram_connect sets unix_peer(sk) without going through that path,
> so a connected AF_UNIX datagram socket has unix_peer(sk) set while
> ctx->peer is still NULL, and the first write that needs revalidation
> reaches the helpers above.
> 
> Both derefs date back to the Fixes: commit, but the update_sk_ctx() one
> was dormant until commit 4483efe4f215 ("apparmor: fix shadowing of plabel
> that prevents cache from being updated") stopped @plabel being shadowed,
> which is why bisecting the oops lands there.
> 
> A NULL @old just means no peer label has been recorded yet, so install
> the label directly instead of merging or comparing against it.
> 
> Fixes: 88fec3526e84 ("apparmor: make sure unix socket labeling is correctly updated.")
> Reported-by: Aurelien Jarno <aurelien@aurel32.net>
> Closes: https://bugs.debian.org/1145111
> Cc: stable@vger.kernel.org
> Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>

FWIW, this triggers when running rustc's test suite on a system with AA
enabled, and I can confirm that the patch fixes the issue/prevents an
oops.

Tested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

> ---
>  security/apparmor/af_unix.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
> index b908e744818c..d04d9cd268aa 100644
> --- a/security/apparmor/af_unix.c
> +++ b/security/apparmor/af_unix.c
> @@ -682,7 +682,7 @@ static void update_sk_ctx(struct sock *sk, struct aa_label *label,
>  		if (old == plabel) {
>  			rcu_assign_pointer(ctx->peer_lastupdate,
>  					   aa_get_label(plabel));
> -		} else if (aa_label_is_subset(plabel, old)) {
> +		} else if (!old || 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));
> @@ -700,13 +700,17 @@ static void update_peer_ctx(struct sock *sk, struct aa_sk_ctx *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);
> -	if (l) {
> -		if (l != old) {
> -			rcu_assign_pointer(ctx->peer, l);
> -			aa_put_label(old);
> -		} else
> -			aa_put_label(l);
> +	if (!old)
> +		rcu_assign_pointer(ctx->peer, aa_get_label(label));
> +	else {
> +		l = aa_label_merge(old, label, GFP_ATOMIC);
> +		if (l) {
> +			if (l != old) {
> +				rcu_assign_pointer(ctx->peer, l);
> +				aa_put_label(old);
> +			} else
> +				aa_put_label(l);
> +		}
>  	}
>  	spin_unlock(&unix_sk(sk)->lock);
>  }
> -- 
> 2.51.0
> 
> 
> 
> 


      parent reply	other threads:[~2026-09-30 10:20 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-24 15:58 [PATCH] apparmor: fix NULL ctx->peer derefs in unix socket ctx updates Maxime Bélair
2026-08-24 21:32 ` Aurelien Jarno
2026-08-31 20:25   ` Aurelien Jarno
2026-09-25 21:41     ` Salvatore Bonaccorso
2026-09-30 10:19 ` Fabian Grünbichler [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1790762923.ps091h0nv7.astroid@yuna.none \
    --to=f.gruenbichler@proxmox.com \
    --cc=apparmor@lists.ubuntu.com \
    --cc=aurelien@aurel32.net \
    --cc=georgia.garcia@canonical.com \
    --cc=john.johansen@canonical.com \
    --cc=linux-security-module@vger.kernel.org \
    --cc=maxime.belair@canonical.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.