[PATCH] apparmor: handle NULL peer label in AF_UNIX context updates

[Maciek Borzecki]

When a confined process performs the first file permission revalidation on
an AF_UNIX socket, both update_sk_ctx() and update_peer_ctx() can be called
before the socket's peer label cache (ctx->peer) has been populated. In
those cases they pass NULL as a label argument to aa_label_is_subset() or
aa_label_merge(), which immediately dereferences label->size and causes an
Oops:

  RIP: __aa_label_next_not_in_set+0xd/0x110
  CR2: 000000000000004c
  Call Trace:
    aa_label_is_subset
    aa_unix_file_perm
    aa_file_perm
    apparmor_file_permission
    security_file_permission
    rw_verify_area
    vfs_write
    ksys_write

Observed on Arch Linux kernels 7.2.4-arch1-2 and 7.2.6-arch2-1, triggered
by snapd unit tests writing to a connected AF_UNIX socket. The issue
persists across those stable updates.

Why this can happen:

* apparmor_socket_socketpair() calls unix_connect_peers(), which pre-
  populates ctx->peer for AF_UNIX socketpairs. Because of that, simple
  socketpair() tests never exercise the NULL-peer path.

* For an AF_UNIX SOCK_DGRAM socket that is connected explicitly (or via
  any path that does not go through unix_connect_peers()), ctx->peer is
  still NULL the first time aa_unix_file_perm() runs. The permission check
  succeeds, then update_peer_ctx() tries to merge label into ctx->peer and
  update_sk_ctx() calls aa_label_is_subset(plabel, ctx->peer). Both
  dereference NULL.

The following reproducer profile demonstrates the bug:

    profile aa_unix_server /path/to/repro {
        #include <abstractions/base>

        change_profile -> aa_unix_client,
        file,
        unix,
        /path/to/repro rmix,
    }

    profile aa_unix_client {
        #include <abstractions/base>

        file,
        unix,
    }

Run a program that binds an abstract AF_UNIX SOCK_DGRAM socket under the
server profile, then forks a child that changes into the aa_unix_client
profile, connects the DGRAM socket to the abstract address, and writes().
The write() path reaches the context-update code with ctx->peer == NULL and
oopses.

Fix all spots in update_sk_ctx() and update_peer_ctx() that can see a NULL
ctx->peer:

1. update_sk_ctx() RCU check: only call aa_label_is_subset() when
   ctx->peer is non-NULL. If it is still NULL, an update is required.

2. update_sk_ctx() spin-locked section: treat a NULL old peer label as
   "plabel is a superset", i.e. populate ctx->peer with plabel.

3. update_peer_ctx(): if ctx->peer is currently NULL, just store the new
   label directly instead of trying to merge with NULL.

Fixes: 88fec3526e84 ("apparmor: make sure unix socket labeling is correctly updated.")
Signed-off-by: Maciek Borzecki <maciek.borzecki@xxxxxxxxx>
---
 security/apparmor/af_unix.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c
index b908e744818c9bfef1638c6abadec75c241dc9b9..4762b70cdc43b5dac87f079067eca0ca74d5dd52 100644
--- a/security/apparmor/af_unix.c
+++ b/security/apparmor/af_unix.c
@@ -659,7 +659,8 @@ static void update_sk_ctx(struct sock *sk, struct aa_label *label,
 	rcu_read_lock();
 	update_sk = (plabel &&
 		     (plabel != rcu_access_pointer(ctx->peer_lastupdate) ||
-		      !aa_label_is_subset(plabel, rcu_dereference(ctx->peer)))) ||
+		      (rcu_access_pointer(ctx->peer) &&
+		       !aa_label_is_subset(plabel, rcu_dereference(ctx->peer))))) ||
 	  !__aa_subj_label_is_cached(label, rcu_dereference(ctx->label));
 	rcu_read_unlock();
 	if (!update_sk)
@@ -682,7 +683,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 +701,18 @@ 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.55.0