Re: [PATCH] apparmor: fix NULL peer label deref for connected SOCK_DGRAM sockets

[John Johansen <john.johansen@canonical.com>]

On 10/20/26 10:30, Nik Tsytsarkin wrote:
> apparmor_sk_alloc_security() initializes ctx->peer and
> ctx->peer_lastupdate to NULL, and unix_connect_peers() is the only
> place they are set.  It is called from the unix_stream_connect and
> socket_socketpair hooks, but not for SOCK_DGRAM sockets paired with
> connect(2): the dgram connect path only goes through the unix_may_send
> hook, which does not touch the socket's apparmor context.  So after
> connect(2) on an AF_UNIX SOCK_DGRAM socket, unix_peer(sk) is set while
> ctx->peer stays NULL.
> 
> When a confined task later triggers file revalidation on such a
> socket (its first write(2) on the fd suffices, with or without a
> profile change in between), aa_unix_file_perm() runs the post-check
> cache update with peer_sk != NULL and a NULL cached peer label, and
> both update paths dereference the NULL label:
> 
>    - update_peer_ctx() calls aa_label_merge(old, label, ...) with
>      old == NULL, which reads old->size;
>    - update_sk_ctx() calls aa_label_is_subset(plabel, old) with
>      old == NULL, which reads old->size via
>      __aa_label_next_not_in_set().
> 
> The AA_BUG() NULL assertions in those helpers compile out without
> CONFIG_SECURITY_APPARMOR_DEBUG_ASSERTS, so production builds oops
> under spin_lock(&unix_sk(sk)->lock).  An unprivileged confined task
> can trigger this whenever its profile mediates unix sockets (complain
> mode included).  Because the oops happens under the unix sock
> spinlock, the dying task then deadlocks against itself in
> unix_release_sock() on exit, leaving an unkillable spinning task and
> an RCU stall on that CPU (and a full crash with panic_on_oops=1).
> 
>    BUG: kernel NULL pointer dereference, address: 000000000000004c
>    RIP: 0010:aa_label_merge+0x63/0x730
>    Call Trace:
>     aa_unix_file_perm+0x477/0x800
>     aa_file_perm+0x434/0x550
>     common_file_perm+0x58/0x150
>     security_file_permission+0x22/0x50
>     vfs_write+0x76/0x410
>     ksys_write+0xb3/0xd0
> 
> Treat a NULL cached peer label as "not initialized yet" in both
> update helpers: have update_peer_ctx() adopt the checked label
> instead of merging with the missing old one, and have update_sk_ctx()
> initialize ctx->peer/ctx->peer_lastupdate when no peer label was
> recorded.  This lazily completes, at first revalidation, the peer
> cross-referencing that unix_connect_peers() performs for stream and
> socketpair sockets.
> 
> Fixes: c05e705812d1 ("apparmor: add fine grained af_unix mediation")
> Assisted-by: LLM
> Signed-off-by: Nik Tsytsarkin (nikt@fb.com)

Hi Nik, thankyou for the patch. This issue has already been addressed
in the apparmor tree by

f61da65fd09e3 apparmor: fix NULL ctx->peer derefs in unix socket ctx updates

> ---
>   security/apparmor/af_unix.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
> index b908e744818c..e802538b0e54 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,7 +700,11 @@ 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 (old)
> + l = aa_label_merge(old, label, GFP_ATOMIC);
> + else
> + /* connected dgram socket, peer ctx was never initialized */
> + l = aa_get_label(label);
>    if (l) {
>    if (l != old) {
>    rcu_assign_pointer(ctx->peer, l);
> --