From 4de32fd05ce9e63b492899215ea929e7d306cdab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Mon, 24 Aug 2026 17:58:02 +0200 Subject: [PATCH 01/23] apparmor: fix NULL ctx->peer derefs in unix socket ctx updates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.") Cc: stable@vger.kernel.org Reported-by: Aurelien Jarno Closes: https://bugs.debian.org/1145111 Tested-by: Fabian Grünbichler Reviewed-by: Georgia Garcia Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- 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..58c638830afc 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); } From a913281f600157aa036b7c02afbb18a0ac115187 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Fri, 16 Oct 2026 19:08:51 -0300 Subject: [PATCH 02/23] apparmor: don't revalidate inherited files under files->file_lock aa_inherit_files() revalidates inherited files on exec using iterate_fd(), which calls the callback with files->file_lock held, so the revalidation is done with IN_ATOMIC. However the unix socket mediation path can sleep: aa_unix_file_perm() drops a ref on the peer's path with path_put(), and unix_fs_perm() -> aa_path_perm() allocates a buffer with aa_get_buffer(false), resulting in BUG: sleeping function called from invalid context at fs/dcache.c:1039 ... dput.part.0+0x24/0x180 path_put+0x2c/0x60 aa_unix_file_perm+0x42a/0x17b0 aa_file_perm+0xcd3/0x1060 match_file+0x10b/0x190 iterate_fd+0xe6/0x280 aa_inherit_files+0x250/0x4f0 Only grab a ref on each file under iterate_fd(), and revalidate it after the lock is dropped, so revalidation is done with no locks held and does not need to be atomic. Denied fds are still replaced with /dev/null, or closed if it can not be opened. Fixes: b1aea2c19607 ("apparmor: fix race in unix socket mediation when peer_path is used") Cc: stable@vger.kernel.org Signed-off-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/file.c | 59 ++++++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 3e74b613db32..b52781e05a5a 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -706,49 +706,50 @@ static void revalidate_tty(const struct cred *subj_cred, struct aa_label *label) no_tty(); } -struct cred_label { - const struct cred *cred; - struct aa_label *label; -}; - -static int match_file(const void *p, struct file *file, unsigned int fd) +/* + * iterate_fd() callback, called with files->file_lock held: grab a ref + * on the next file so it can be revalidated without the lock held. + */ +static int get_next_file(const void *p, struct file *file, unsigned int fd) { - struct cred_label *cl = (struct cred_label *)p; + *(struct file **)p = get_file(file); - if (aa_file_perm(OP_INHERIT, cl->cred, cl->label, file, - aa_map_file_to_perms(file), IN_ATOMIC)) - return fd + 1; - return 0; + return fd + 1; } - /* based on selinux's flush_unauthorized_files */ void aa_inherit_files(const struct cred *cred, struct files_struct *files) { struct aa_label *label = aa_get_newest_cred_label(cred); - struct cred_label cl = { - .cred = cred, - .label = label, - }; - struct file *devnull = NULL; - unsigned int n; + struct file *devnull = NULL, *file; + bool devnull_tried = false; + unsigned int n = 0; revalidate_tty(cred, label); - /* Revalidate access to inherited open files. */ - n = iterate_fd(files, 0, match_file, &cl); - if (!n) /* none found? */ - goto out; + /* + * Revalidate access to inherited open files. Each file is + * revalidated holding only a reference to it, with no locks + * held, as revalidation may sleep (e.g. unix socket mediation + * dropping path refs, or allocating path buffers). + */ + while ((n = iterate_fd(files, n, get_next_file, &file)) != 0) { + int error = aa_file_perm(OP_INHERIT, cred, label, file, + aa_map_file_to_perms(file), false); - devnull = dentry_open(&aa_null, O_RDWR, cred); - if (IS_ERR(devnull)) - devnull = NULL; - /* replace all the matching ones with this */ - do { + fput(file); + if (!error) + continue; + if (!devnull_tried) { + devnull_tried = true; + devnull = dentry_open(&aa_null, O_RDWR, cred); + if (IS_ERR(devnull)) + devnull = NULL; + } + /* replace the denied fd (n - 1) with devnull, or close it */ replace_fd(n - 1, devnull, 0); - } while ((n = iterate_fd(files, n, match_file, &cl)) != 0); + } if (devnull) fput(devnull); -out: aa_put_label(label); } From 049760746597cfd054c181fe850531be3c1e7358 Mon Sep 17 00:00:00 2001 From: shaos Date: Tue, 20 Oct 2026 00:12:56 +0000 Subject: [PATCH 03/23] apparmor: mediate AF_UNSPEC socket disconnects aa_inet_connect_perm() returns success immediately for AF_UNSPEC before checking the active AppArmor profile. AF_UNSPEC is the socket disconnect operation for TCP and UDP. A process confined by a profile containing deny network can therefore disconnect inherited sockets even though ordinary socket creation and connect calls are denied. console output: https://gist.githubusercontent.com/shaos26/73b057c77b9ea22d5e6813c259dbf43c/raw/fab5c58d6f184f5fab88a3a31a80c2b919efecf0/console-output.txt reproduction kernel config: https://gist.githubusercontent.com/shaos26/73b057c77b9ea22d5e6813c259dbf43c/raw/b9192c97c92fdd3b59089704bad6fbd0a33ccf06/kernel-config.txt reproducer: https://gist.githubusercontent.com/shaos26/73b057c77b9ea22d5e6813c259dbf43c/raw/92c7dd098a227276fe3daeea45996e2848e66439/poc.c Build the kernel from HEAD with the attached reproduction config, then make a one-file initramfs. The PoC mounts sysfs and securityfs itself; DEVTMPFS_MOUNT supplies /dev/console. mkdir -p build rootfs cp kernel-config.txt build/.config make O=build olddefconfig make O=build -j"$(nproc)" bzImage cc -O2 -static -Wall -Wextra -o poc poc.c cp poc rootfs/poc (cd rootfs && find poc | cpio -o -H newc --owner=0:0) | gzip -9 > initramfs.cpio.gz timeout 900 qemu-system-x86_64 -kernel build/arch/x86/boot/bzImage \ -initrd initramfs.cpio.gz -append "console=ttyS0 rdinit=/poc panic=-1 loglevel=7 lsm=apparmor security=apparmor apparmor=1" \ -nographic -m 1024 -smp 1 -no-reboot 2>&1 | tee console.log Under the deny-network profile, socket() and connect(AF_INET) returned EACCES, but connect(fd, AF_UNSPEC) returned success and tore down the inherited connection and listener. Two fresh boots matched. A one-line control that routed AF_UNSPEC through aa_sk_perm returned EACCES and kept both sockets alive. Impact and prerequisites: A confined process that inherits a TCP/UDP socket can bypass the profile's network mediation for disconnect and disrupt that socket. The finding does not permit socket creation, bind, or arbitrary peer connections; no privilege escalation is demonstrated. Fixes: 89ac6aa752ec8 ("apparmor: push inet mediation into profile callbacks, and improve auditing") Signed-off-by: shaos Signed-off-by: John Johansen --- security/apparmor/af_inet.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/af_inet.c b/security/apparmor/af_inet.c index 516c9016537c..42994e84ccca 100644 --- a/security/apparmor/af_inet.c +++ b/security/apparmor/af_inet.c @@ -425,7 +425,7 @@ int aa_inet_connect_perm(struct socket *sock, struct sockaddr *addr, if (addrlen < offsetofend(struct sockaddr, sa_family)) return -EINVAL; if (addr->sa_family == AF_UNSPEC) - return 0; + return aa_sk_perm(OP_CONNECT, AA_MAY_CONNECT, sock->sk); /* do we need early bailout for !family ... */ return sk_has_perm2(sock->sk, OP_CONNECT, AA_MAY_CONNECT, profile, ad, From 4890a8b1a9df880dd188adb8b8e8fab107ad69d9 Mon Sep 17 00:00:00 2001 From: Michail Andreas Michalopoulos Date: Tue, 20 Oct 2026 16:00:30 +0200 Subject: [PATCH 04/23] apparmor: check address length before reading AF_UNSPEC bind address bind_map_addr() handles AF_UNSPEC on an IPv4 socket by reading addr4->sin_addr.s_addr to check for INADDR_ANY. At that point the only length check done is addrlen >= offsetofend(struct sockaddr, sa_family), i.e. 2 bytes, while sin_addr sits at offset 4. move_addr_to_kernel() copies only addrlen bytes into the on-stack sockaddr_storage, so a bind() with a short AF_UNSPEC address makes AppArmor branch on bytes the caller never supplied. Depending on those bytes, a confined task gets -EAFNOSUPPORT instead of the -EINVAL that inet_bind_sk() returns for the same request. Kernels built with CONFIG_INIT_STACK_ALL_ZERO read zeroes and happen to behave correctly. Check the length first, as inet_bind_sk() does, and as the AF_INET6 case above already does for inet6_bind_sk(). Fixes: 89ac6aa752ec ("apparmor: push inet mediation into profile callbacks, and improve auditing") Assisted-by: LLM Signed-off-by: Michail Andreas Michalopoulos Signed-off-by: John Johansen --- security/apparmor/af_inet.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/security/apparmor/af_inet.c b/security/apparmor/af_inet.c index 42994e84ccca..2a933572fa95 100644 --- a/security/apparmor/af_inet.c +++ b/security/apparmor/af_inet.c @@ -196,6 +196,9 @@ static int bind_map_addr(const struct sock *sk, struct sockaddr *addr, * AF_UNSPEC if the address is INADDR_ANY */ addr4 = (struct sockaddr_in *)addr; + /* Length check from inet_bind_sk() */ + if (addrlen < sizeof(struct sockaddr_in)) + return -EINVAL; if (addr4->sin_addr.s_addr != htonl(INADDR_ANY)) return -EAFNOSUPPORT; family = AF_INET; From 33ed50716251d5b2875706c40d0c8082d46be0b1 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 20 Oct 2026 16:00:30 -0700 Subject: [PATCH 05/23] apparmor: fix refcount leak in aa_get_newest_profile() The labels_profile() macro is evaluating its argument twice #define labels_profile(X) ((X)->vec[(X)->size - 1]) unfortunately aa_get_newest_profile() passes aa_get_newest_label(&p->label) so every call to aa_get_newest_profile results in 2 refcounts being taken with one being leaked. There is no need for labels_profile() to be a macro. Convert it to an inline fn, which ensures all uses are passing the correct struct and removes the double evaluation. Fixes: 637f688dc3dc ("apparmor: switch from profiles to using labels on contexts") Cc: stable@vger.kernel.org Reported-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/include/label.h | 6 +++++- security/apparmor/label.c | 1 - 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/security/apparmor/include/label.h b/security/apparmor/include/label.h index 2c869b7dec97..61794b9cd6be 100644 --- a/security/apparmor/include/label.h +++ b/security/apparmor/include/label.h @@ -162,7 +162,11 @@ do { \ #define labels_ns(X) (vec_ns(&((X)->vec[0]), (X)->size)) #define labels_set(X) (&labels_ns(X)->labels) #define labels_view(X) labels_ns(X) -#define labels_profile(X) ((X)->vec[(X)->size - 1]) + +static inline struct aa_profile *labels_profile(const struct aa_label *label) +{ + return label->vec[label->size - 1]; +} int aa_label_next_confined(const struct aa_label *l, int i); diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 82742a471055..42ed476fe89d 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -387,7 +387,6 @@ void aa_label_kref(struct kref *kref) label_free_switch(label); return; } - /* TODO: update labels_profile macro so it works here */ AA_BUG(label_isprofile(label) && on_list_rcu(&label->vec[0]->base.profiles)); AA_BUG(label_isprofile(label) && From 1f5428b9f9bfb9dab6828015a5254dcd7c582a01 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 20 Oct 2026 12:23:13 -0700 Subject: [PATCH 06/23] apparmor: remove unconfined_profile() check in inet socket mediation When a stacked label is used the early bailout !unconfined(label) fails, and the individual profile components of the label are checked. This can lead to aa_profile_af_perm() being called which results in WARNING: security/apparmor/net.c:263 at aa_profile_af_perm+0x228/0x3b0 on any inet socket using basic socket mediation. Remove the AA_BUG(profile_unconfined(profile)) check, it is not needed as the code already handles the unconfined_profile() case. In addition also a comment to indicate that this case is covered. Fixes: 89ac6aa752ec ("apparmor: push inet mediation into profile callbacks, and improve auditing") Reported-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/net.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/security/apparmor/net.c b/security/apparmor/net.c index a333e6aff926..641d181544ac 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -260,7 +260,6 @@ int aa_profile_af_perm(struct aa_profile *profile, AA_BUG(family >= AF_MAX); AA_BUG(type < 0 || type >= SOCK_MAX); - AA_BUG(profile_unconfined(profile)); state = RULE_MEDIATES_NET(rules); if (state) { @@ -268,7 +267,7 @@ int aa_profile_af_perm(struct aa_profile *profile, type, protocol, &p, &ad->info); return aa_do_perms(profile, rules->policy, state, request, p, ad); - } /* else */ + } /* else - not mediated this covers the profile_unconfined() case */ return 0; } From f7bd6285ffeb425793334f6d48ccb5035cab2fa7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Wed, 19 Aug 2026 14:19:44 +0100 Subject: [PATCH 07/23] apparmor: fix a leak on a rejected string table entry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit unpack_strs_table() stores each entry into the table only after validating it, so an entry the validation rejects is never freed. Up to 64KB leaks per rejected entry. Store the pointer before validating it. Fixes: c140dcd1246b ("apparmor: make str table more generic and be able to have multiple entries") Reviewed-by: Ryan Lee Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index f1fc48e72d0e..99bc4f946d15 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -558,6 +558,9 @@ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, /* aa_unpack_strdup verifies that the last character is * null termination byte. */ + /* publish before validating; the error path frees it */ + table[i].strs = str; + table[i].size = size2; c = process_strs_entry(str, size2, multi); if (c <= 0) { AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", @@ -570,9 +573,7 @@ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, /* fail - all other cases with embedded \0 */ goto fail; } - table[i].strs = str; table[i].count = c; - table[i].size = size2; } if (!aa_unpack_nameX(e, AA_ARRAYEND, NULL)) goto fail; From 07aee062c6ecaa7199be583c83a1c74ae9885f03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Wed, 19 Aug 2026 10:00:19 +0100 Subject: [PATCH 08/23] apparmor: report a label allocation failure as -ENOMEM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit aa_label_strn_parse() returns -ENOENT for every failure, so a caller cannot tell "no such profile" from "could not allocate a label". Report an allocation failure as -ENOMEM. Fixes: f1bd904175e8 ("apparmor: add the base fns() for domain labels") Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- security/apparmor/label.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 42ed476fe89d..fd67262330f9 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1954,12 +1954,18 @@ struct aa_label *aa_label_strn_parse(struct aa_label *base, const char *str, goto out; } - if (create) + if (create) { label = aa_vec_find_or_create_label(vec, len, gfp); - else + if (!label) { + /* not absence: creating only fails on allocation */ + label = ERR_PTR(-ENOMEM); + goto out; + } + } else { label = vec_find(vec, len); - if (!label) - goto fail; + if (!label) + goto fail; + } out: /* use adjusted len from after vec_unique, not original */ From 7ccd884a5e5386f4c7d98625113f4924f70286b4 Mon Sep 17 00:00:00 2001 From: Li RongQing Date: Thu, 8 Oct 2026 11:15:33 +0800 Subject: [PATCH 09/23] apparmor: avoid potential double free when remapping DFA tables When unpacking a DFA, aa_dfa_unpack() stores a table in dfa->tables before attempting to remap a 16-bit table to 32-bit. However, remap_data16_to_data32() frees the original table when allocation of the new table fails. This leaves the corresponding dfa->tables entry pointing to freed memory. If the unpacking then fails, dfa_free() attempts to free the same table again. Only publish the table to dfa->tables after any required remapping has completed successfully, so that dfa->tables never contains a pointer to a table that has already been freed. Fixes: 9208c05f9fdf ("apparmor: add support for 2^24 states to the dfa state machine.") Cc: stable@vger.kernel.org Signed-off-by: Li RongQing Signed-off-by: John Johansen --- security/apparmor/match.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/security/apparmor/match.c b/security/apparmor/match.c index 7713484f6a36..ca30a4292c42 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -397,7 +397,6 @@ struct aa_dfa *aa_dfa_unpack(const void *blob, size_t size, int flags) /* check for duplicate table entry */ if (dfa->tables[table->td_id]) goto fail; - dfa->tables[table->td_id] = table; data += table_size(table->td_lolen, table->td_flags); size -= table_size(table->td_lolen, table->td_flags); @@ -414,9 +413,9 @@ struct aa_dfa *aa_dfa_unpack(const void *blob, size_t size, int flags) if (!table) goto fail; } - dfa->tables[table->td_id] = table; break; } + dfa->tables[table->td_id] = table; table = NULL; } error = verify_table_headers(dfa->tables, flags); From 449195a4bb7d9c456c02b871eec6905edb449bd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Thu, 3 Sep 2026 09:25:51 +0200 Subject: [PATCH 10/23] apparmor: fix table_size() integer overflow on 32 bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit table_size() checks ALIGN(sizeof(struct table_header) + len * el_size, 8) in size_t, where @len is td_lolen, a u32 user-provided value from the policy blob. On 32 bit kernels both operations can overflow. Triggering this bug allows to: - overflow a 16 byte allocation with up to 4GiB of policy data - store the table header at address ZERO_SIZE_PTR (kvzalloc(0)) However, this bug requires CAP_MAC_ADMIN, only affects 32 bit kernels, and the copy runs until the machine is destroyed, so it gives no controllable primitive and is therefore not a security issue. Compute the size with the checked helpers and return 0 when the result is not representable, so the callers can reject the table. A valid table is never 0 bytes, the header alone is 12. 64 bit kernels cannot overflow (these structs are 32 bit), and are hence not affected. Fixes: e06f75a6a2b4 ("AppArmor: dfa match engine") Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- security/apparmor/include/match.h | 11 ++++++++++- security/apparmor/match.c | 6 +++++- security/apparmor/policy_unpack.c | 5 +++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h index f7bd7855f1bd..66485f76d331 100644 --- a/security/apparmor/include/match.h +++ b/security/apparmor/include/match.h @@ -12,6 +12,7 @@ #define __AA_MATCH_H #include +#include #define DFA_NOMATCH 0 #define DFA_START 1 @@ -120,7 +121,15 @@ struct aa_dfa { static inline size_t table_size(size_t len, size_t el_size) { - return ALIGN(sizeof(struct table_header) + len * el_size, 8); + size_t size; + + /* Need check_*_overflow for 32 bit machines */ + if (check_mul_overflow(len, el_size, &size) || + check_add_overflow(size, sizeof(struct table_header), &size) || + size > SIZE_MAX - 7) + return 0; + + return ALIGN(size, 8); } #define aa_state_t unsigned int diff --git a/security/apparmor/match.c b/security/apparmor/match.c index ca30a4292c42..31b309043583 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -58,7 +58,7 @@ static struct table_header *unpack_table(const char *blob, size_t bsize) if (th.td_lolen == 0) goto out; tsize = table_size(th.td_lolen, th.td_flags); - if (bsize < tsize) + if (!tsize || bsize < tsize) goto out; table = kvzalloc(tsize, GFP_KERNEL); @@ -281,6 +281,10 @@ static struct table_header *remap_data16_to_data32(struct table_header *old) u32 i; tsize = table_size(old->td_lolen, YYTD_DATA32); + if (!tsize) { + kvfree(old); + return NULL; + } new = kvzalloc(tsize, GFP_KERNEL); if (!new) { kvfree(old); diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 99bc4f946d15..3ffbab46eec3 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1053,6 +1053,11 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, u16 tdflags = pdb->dfa->tables[YYTD_ID_ACCEPT]->td_flags; size_t tsize = table_size(noents, tdflags); + if (!tsize) { + *info = "invalid dfa flags table size"; + error = -EPROTO; + goto fail; + } pdb->dfa->tables[YYTD_ID_ACCEPT2] = kvzalloc(tsize, GFP_KERNEL); if (!pdb->dfa->tables[YYTD_ID_ACCEPT2]) { *info = "failed to alloc dfa flags table"; From e667fc72f7f4a54674962c324048ca2cc58bc703 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Tue, 1 Sep 2026 16:07:15 +0200 Subject: [PATCH 11/23] apparmor: convert duplicated dfa transitions to match_char() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For historical reasons several matchers duplicate the dfa character transition instead of using match_char(). This is because aa_dfa_match_until() and aa_dfa_matchn_until() were written on a branch that did not yet have match_char(), and copied code from aa_dfa_match(). leftmatch_fb() used the same pattern. When match_char() added support for diff-encoded states, the change was never ported to these copies. match_char() resolves a diff encoded state by using def[state] and retrying the same character, until it reaches a state with a valid transition. The copies instead do a simple `state = def[state];` after the character is consumed, so on a diff encoded state one byte is dropped and the walk continues from the diff parent in the wrong state. This makes an unnamed px/Px/cx/Cx transition resolve to no profile, and exec fails with apparmor="DENIED" operation="exec" info="profile transition not found" Convert the three copies to match_char(). For a dfa with no diff encoded state this is a no-op. This bug is latent until the kernel advertises policy/diff-encode. Closes: https://gitlab.com/apparmor/apparmor/-/work_items/663 Fixes: 21f606610502 ("apparmor: improve overlapping domain attachment resolution") Fixes: 031dcc8f4e84f ("apparmor: dfa add support for state differential encoding") Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- security/apparmor/match.c | 52 ++++++++++++++------------------------- 1 file changed, 19 insertions(+), 33 deletions(-) diff --git a/security/apparmor/match.c b/security/apparmor/match.c index 31b309043583..cf3b35a6b1cd 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -617,7 +617,7 @@ aa_state_t aa_dfa_match_until(const struct aa_dfa *dfa, aa_state_t start, u32 *next = NEXT_TABLE(dfa); u32 *check = CHECK_TABLE(dfa); u32 *accept = ACCEPT_TABLE(dfa); - aa_state_t state = start, pos; + aa_state_t state = start; if (state == DFA_NOMATCH) return DFA_NOMATCH; @@ -628,22 +628,18 @@ aa_state_t aa_dfa_match_until(const struct aa_dfa *dfa, aa_state_t start, u8 *equiv = EQUIV_TABLE(dfa); /* default is direct to next state */ while (*str) { - pos = base_idx(base[state]) + equiv[(u8) *str++]; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + u8 c = equiv[(u8) *str++]; + + match_char(state, def, base, next, check, c); if (accept[state]) break; } } else { /* default is direct to next state */ while (*str) { - pos = base_idx(base[state]) + (u8) *str++; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + u8 c = (u8) *str++; + + match_char(state, def, base, next, check, c); if (accept[state]) break; } @@ -678,7 +674,7 @@ aa_state_t aa_dfa_matchn_until(const struct aa_dfa *dfa, aa_state_t start, u32 *next = NEXT_TABLE(dfa); u32 *check = CHECK_TABLE(dfa); u32 *accept = ACCEPT_TABLE(dfa); - aa_state_t state = start, pos; + aa_state_t state = start; *retpos = NULL; if (state == DFA_NOMATCH) @@ -690,22 +686,18 @@ aa_state_t aa_dfa_matchn_until(const struct aa_dfa *dfa, aa_state_t start, u8 *equiv = EQUIV_TABLE(dfa); /* default is direct to next state */ for (; n; n--) { - pos = base_idx(base[state]) + equiv[(u8) *str++]; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + u8 c = equiv[(u8) *str++]; + + match_char(state, def, base, next, check, c); if (accept[state]) break; } } else { /* default is direct to next state */ for (; n; n--) { - pos = base_idx(base[state]) + (u8) *str++; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + u8 c = (u8) *str++; + + match_char(state, def, base, next, check, c); if (accept[state]) break; } @@ -754,7 +746,7 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start, u32 *base = BASE_TABLE(dfa); u32 *next = NEXT_TABLE(dfa); u32 *check = CHECK_TABLE(dfa); - aa_state_t state = start, pos; + aa_state_t state = start; AA_BUG(!dfa); AA_BUG(!str); @@ -772,13 +764,10 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start, /* default is direct to next state */ while (*str) { unsigned int adjust; + u8 c = equiv[(u8) *str++]; wb->history[wb->pos] = state; - pos = base_idx(base[state]) + equiv[(u8) *str++]; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + match_char(state, def, base, next, check, c); if (is_loop(wb, state, &adjust)) { state = aa_dfa_match(dfa, state, str); *count -= adjust; @@ -791,13 +780,10 @@ static aa_state_t leftmatch_fb(const struct aa_dfa *dfa, aa_state_t start, /* default is direct to next state */ while (*str) { unsigned int adjust; + u8 c = (u8) *str++; wb->history[wb->pos] = state; - pos = base_idx(base[state]) + (u8) *str++; - if (check[pos] == state) - state = next[pos]; - else - state = def[state]; + match_char(state, def, base, next, check, c); if (is_loop(wb, state, &adjust)) { state = aa_dfa_match(dfa, state, str); *count -= adjust; From fcd621debff51f09cc665e1063a6025d7c1bfd48 Mon Sep 17 00:00:00 2001 From: Karl Mehltretter Date: Sat, 12 Sep 2026 02:03:02 +0200 Subject: [PATCH 12/23] apparmor: Fix the return value in split_token_from_name() kernel-doc split_token_from_name() returns ERR_PTR(-EINVAL) for malformed input, but its kernel-doc describes NULL as the failure result. NULL is instead a successful result when the name following the token is empty. Document all three results: the name, NULL for an empty name, and an error pointer for malformed input. Assisted-by: LLM Signed-off-by: Karl Mehltretter Signed-off-by: John Johansen --- security/apparmor/procattr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/apparmor/procattr.c b/security/apparmor/procattr.c index c07b6e8fd9c9..cfc45db43aa0 100644 --- a/security/apparmor/procattr.c +++ b/security/apparmor/procattr.c @@ -74,7 +74,8 @@ int aa_getprocattr(struct aa_label *label, char **string, bool newline) * @args: string to parse (NOT NULL) * @token: stores returned parsed token value (NOT NULL) * - * Returns: start position of name after token else NULL on failure + * Returns: the name following the token, %NULL if the name is empty, or + * ERR_PTR(-EINVAL) if the input is malformed. */ static char *split_token_from_name(const char *op, char *args, u64 *token) { From afe5d5a747393cb3d00795db31d7fce5108a766a Mon Sep 17 00:00:00 2001 From: Guanglei Zhu Date: Wed, 16 Sep 2026 19:53:07 +0800 Subject: [PATCH 13/23] apparmor: check accept index + 1 is in bounds aa_lookup_condperms() returns perms[index + 1] when an accept entry has ACCEPT_FLAG_OWNER set and the subject uid does not match the file uid, so such an entry consumes two slots in the perms table. But verify_dfa_accept_index() only checks that index is within the perms table, so a policy that sets the owner flag on the last entry is accepted, and the runtime lookup then reads perms[index + 1] one entry past the end of the table. The perms table is allocated by unpack_perms_table() with a u16 size read from the policy blob, so a crafted policy can place the owner flag at index size - 1 to reach the out-of-bounds read. Check that index + 1 is also in bounds when the owner flag is set. Fixes: 6f442d42c0d89 ("apparmor: fix profile verification and enable it") Cc: stable@vger.kernel.org Signed-off-by: Guanglei Zhu Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 3ffbab46eec3..7ebffb4ff74f 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1495,6 +1495,15 @@ static bool verify_dfa_accept_index(const struct aa_dfa *dfa, int table_size) for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) { if (ACCEPT_TABLE(dfa)[i] >= table_size) return false; + /* + * Accept indexes for owner-conditional permissions come in + * pairs, so the non-owner entry at index + 1 must also be + * in bounds. + */ + if (dfa->tables[YYTD_ID_ACCEPT2] && + (ACCEPT_TABLE2(dfa)[i] & ACCEPT_FLAG_OWNER) && + ACCEPT_TABLE(dfa)[i] + 1 >= table_size) + return false; } return true; } From 8f1f7f664c1822af34c5465f9820b4e39146786b Mon Sep 17 00:00:00 2001 From: Wentao Liang Date: Thu, 17 Sep 2026 16:43:02 +0000 Subject: [PATCH 14/23] apparmor: Fix namespace reference leak in __aa_find_or_create_ns() When the namespace already exists, the reference taken by aa_get_ns() is overwritten with ERR_PTR(-EEXIST) without being released. The caller in apparmorfs.c clears ns on IS_ERR() and so cannot drop it either, leaking the reference on raced mkdir of an existing namespace. Release the reference before returning -EEXIST. Fixes: 73688d1ed0b8 ("apparmor: refactor prepare_ns() and make usable from different views") Signed-off-by: Wentao Liang Signed-off-by: John Johansen --- security/apparmor/policy_ns.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c index 5a907a875d8f..45e66b14c2d5 100644 --- a/security/apparmor/policy_ns.c +++ b/security/apparmor/policy_ns.c @@ -265,10 +265,12 @@ struct aa_ns *__aa_find_or_create_ns(struct aa_ns *parent, const char *name, /* try and find the specified ns */ /* released by caller */ ns = aa_get_ns(__aa_find_ns(&parent->sub_ns, name)); - if (!ns) + if (!ns) { ns = __aa_create_ns(parent, name, dir); - else + } else { + aa_put_ns(ns); ns = ERR_PTR(-EEXIST); + } /* return ref */ return ns; From f292873ceb4bd3a1a0f75f884361cd556e457692 Mon Sep 17 00:00:00 2001 From: Binbin Deng <18983559317@163.com> Date: Wed, 23 Sep 2026 13:46:23 +0800 Subject: [PATCH 15/23] apparmor: fix a use-after-free in aa_lookupn_ns() aa_lookupn_ns() takes a reference on the namespace it finds with aa_get_ns(), which is kref_get() on the reference count that the namespace is released through, without checking whether the namespace is still in use. A concurrent removal of a ":"-prefixed namespace looks the same namespace up and only then takes the parent lock to remove it, so the last reference can be dropped between the lookup and the aa_get_ns() call: the reference count then reports the increment on a zero count and the kernel classifies the following use of the object as a use-after-free. A user with CAP_MAC_ADMIN can trigger this by writing a ":" removal and a ":/" removal to /sys/kernel/security/apparmor/.remove concurrently. refcount_t: addition on 0; use-after-free. WARNING: lib/refcount.c:25 at refcount_warn_saturate+0xc0/0xe0, CPU#6 Call Trace: aa_lookupn_ns+0xa9/0xc0 aa_remove_profiles+0x4fb/0x730 ? __kvmalloc_node_noprof+0x219/0x5c0 ? __pfx_aa_remove_profiles+0x10/0x10 ? aa_loaddata_alloc+0x65/0x1f0 ? _copy_from_user+0x2d/0x80 ? aa_loaddata_alloc+0x111/0x1f0 profile_remove+0x2ee/0x4a0 vfs_write+0x21e/0xcf0 ? rcu_core+0x29c/0x1740 ? timerqueue_linked_add+0x1f4/0x3d0 ? __pfx_vfs_write+0x10/0x10 ? mutex_lock+0x81/0xe0 ? __pfx_mutex_lock+0x10/0x10 ? __pfx_rcu_core+0x10/0x10 ? __pfx_hrtimer_update_next_event+0x10/0x10 ? mutex_unlock+0x7b/0xd0 ? fdget_pos+0x24d/0x4b0 ksys_write+0xf7/0x1c0 ? __pfx_ksys_write+0x10/0x10 ? restore_fpregs_from_fpstate+0x37/0xc0 do_syscall_64+0xf9/0x540 entry_SYSCALL_64_after_hwframe+0x77/0x7f Fix by rejecting a namespace whose reference count is zero. Fixes: 3664268f19ea0 ("apparmor: add namespace lookup fns()") Cc: stable@vger.kernel.org Signed-off-by: Binbin Deng <18983559317@163.com> Signed-off-by: John Johansen --- security/apparmor/include/policy_ns.h | 16 ++++++++++++++++ security/apparmor/policy_ns.c | 2 +- 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/security/apparmor/include/policy_ns.h b/security/apparmor/include/policy_ns.h index cc6e84151812..4f8359875476 100644 --- a/security/apparmor/include/policy_ns.h +++ b/security/apparmor/include/policy_ns.h @@ -117,6 +117,22 @@ static inline struct aa_ns *aa_get_ns(struct aa_ns *ns) return ns; } +/** + * aa_get_ns_not0 - increment references count on @ns found via lookup + * @ns: namespace to increment reference count of (MAYBE NULL) + * + * Returns: pointer to @ns, if @ns is NULL returns NULL, + * NULL if @ns is being freed + * Requires: @ns must be held with valid refcount when called + */ +static inline struct aa_ns *aa_get_ns_not0(struct aa_ns *ns) +{ + if (ns && aa_get_profile_not0(ns->unconfined)) + return ns; + + return NULL; +} + /** * aa_put_ns - decrement refcount on @ns * @ns: namespace to put reference of diff --git a/security/apparmor/policy_ns.c b/security/apparmor/policy_ns.c index 45e66b14c2d5..7d5c768493e8 100644 --- a/security/apparmor/policy_ns.c +++ b/security/apparmor/policy_ns.c @@ -207,7 +207,7 @@ struct aa_ns *aa_lookupn_ns(struct aa_ns *view, const char *name, size_t n) struct aa_ns *ns = NULL; rcu_read_lock(); - ns = aa_get_ns(__aa_lookupn_ns(view, name, n)); + ns = aa_get_ns_not0(__aa_lookupn_ns(view, name, n)); rcu_read_unlock(); return ns; From 2f764c08bb191a8c8f4051926dba0d25d195f39b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=A9my=20Jean?= Date: Fri, 25 Sep 2026 20:54:48 +0000 Subject: [PATCH 16/23] apparmor: check connect permission for SCTP MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SCTP can start an association through sendmsg() or CONNECTX. These paths call the SCTP-specific sctp_bind_connect hook instead of the normal socket connect hook, but AppArmor does not register it. As a result, a profile can deny connect and allow send or setopt, yet a task can still create an SCTP association. Register the SCTP hook. Ask for connect permission when SENDMSG_CONNECT or CONNECTX creates an association. Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket mediation") Assisted-by: LLM Signed-off-by: Jérémy Jean Signed-off-by: John Johansen --- security/apparmor/lsm.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index d502ad0ac26f..dba078c73abc 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -15,6 +15,7 @@ #include #include #include +#include #include #include #include @@ -1405,6 +1406,18 @@ static int apparmor_socket_connect(struct socket *sock, return aa_sk_perm(OP_CONNECT, AA_MAY_CONNECT, sock->sk); } +static int apparmor_sctp_bind_connect(struct sock *sk, int optname, + struct sockaddr *address, int addrlen) +{ + switch (optname) { + case SCTP_SOCKOPT_CONNECTX: + case SCTP_SENDMSG_CONNECT: + return aa_sk_perm(OP_CONNECT, AA_MAY_CONNECT, sk); + } + + return 0; +} + static int apparmor_socket_listen(struct socket *sock, int backlog) { AA_BUG(!sock); @@ -1823,6 +1836,7 @@ static struct security_hook_list apparmor_hooks[] __ro_after_init = { LSM_HOOK_INIT(socket_getsockopt, apparmor_socket_getsockopt), LSM_HOOK_INIT(socket_setsockopt, apparmor_socket_setsockopt), LSM_HOOK_INIT(socket_shutdown, apparmor_socket_shutdown), + LSM_HOOK_INIT(sctp_bind_connect, apparmor_sctp_bind_connect), #ifdef CONFIG_NETWORK_SECMARK LSM_HOOK_INIT(socket_sock_rcv_skb, apparmor_socket_sock_rcv_skb), #endif From 7a82f9a06459ff1c37b5e97b204430442cc076e3 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 29 Oct 2026 09:03:55 -0700 Subject: [PATCH 17/23] apparmor: fix check SCTP bind, and connect permission checks SCTP mediation of bind is not being checked correctly because bind permission needs to be checked in the sctp_bind_connect() hook. Require for bind permissions when CTP_SET_PEER_PRIMARY_ADDR, and SCTP_SOCKOPT_BINDX_ADD are asked for. The peer initialized ASCONF cases (SCTP_PARAM_ADD_IP and SCTP_PARAM_SET_PRIMARY) are processed in softirq context and are not mediated atm. SCTP_PRIMATEY_ADDR, and SCTP_SET_PEER_PRIMARY_ADDR only select among address already in the association. Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket mediation") Signed-off-by: John Johansen --- security/apparmor/lsm.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index dba078c73abc..de6633e238f3 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -1410,6 +1410,11 @@ static int apparmor_sctp_bind_connect(struct sock *sk, int optname, struct sockaddr *address, int addrlen) { switch (optname) { + /* Bind checks */ + case SCTP_SET_PEER_PRIMARY_ADDR: + case SCTP_SOCKOPT_BINDX_ADD: + return aa_sk_perm(OP_BIND, AA_MAY_BIND, sk); + /* Connect checks */ case SCTP_SOCKOPT_CONNECTX: case SCTP_SENDMSG_CONNECT: return aa_sk_perm(OP_CONNECT, AA_MAY_CONNECT, sk); From 35b8ab3f36ac0fcd8fc910da0bed889b6c75c426 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 30 Sep 2026 08:49:43 -0700 Subject: [PATCH 18/23] apparmor: advertise that sctp is being mediated Enable the test harness to detect that the kernel should be properly mediating sctp. Fixes: 7a82f9a06459f ("apparmor: fix check SCTP bind, and connect permission checks") Signed-off-by: John Johansen --- security/apparmor/net.c | 1 + 1 file changed, 1 insertion(+) diff --git a/security/apparmor/net.c b/security/apparmor/net.c index 641d181544ac..136d14226b97 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -24,6 +24,7 @@ struct aa_sfs_entry aa_sfs_entry_network[] = { AA_SFS_FILE_STRING("af_mask", AA_SFS_AF_MASK), AA_SFS_FILE_BOOLEAN("tcp-fast-open", 1), + AA_SFS_FILE_BOOLEAN("sctp", 1), { } }; From d30844001360a403a511cf24fc75ecaa6a7a7544 Mon Sep 17 00:00:00 2001 From: "Cen Zhang (Microsoft)" Date: Mon, 12 Oct 2026 17:57:03 -0400 Subject: [PATCH 19/23] apparmor: fix query_data() output overlapping the lookup key In query_data(), buf and query point into the same multi_transaction buffer: buf at the start, query a few bytes in, and the lookup key right after the label string that query points to. Reusing the buffer is by design and is safe as long as the query is fully parsed before buf is written to, but the key is not: it is dereferenced on every iteration of label_for_each_confined(), after output has already been written. So when the label has more than one profile with a data table, the data of the first matching profile is written to buf while the key is still needed, and overwrites it if the data is large enough. The later lookups then run on a corrupted key. Besides, if the data has no NUL byte from the key's position to the end of the allocation, strhash() -> strlen() -> jhash() reads past the 4096-byte slab object, triggering an out-of-bound read: BUG: KASAN: slab-out-of-bounds in jhash+0x595/0x5e0 Read of size 1 at addr ff1100001159d000 by task exploit/155 CPU: 0 UID: 1000 PID: 155 Comm: exploit Not tainted 7.3.0-rc7 #1 Call Trace: jhash+0x595/0x5e0 aa_write_access+0x880/0x1400 vfs_write+0x26f/0xd20 ksys_write+0x115/0x210 Writing to the .access file is unprivileged (0666), but triggering this requires loaded profiles that carry a data table, which needs CAP_MAC_ADMIN and is not the case on default distribution policies. Fix by copying the key with kstrdup() before writing any output to buf, so the key stays valid across all loop iterations regardless of what gets written to the overlapping buffer. Fixes: 317d9a054e1c ("apparmor: update query interface to support label queries") Cc: stable@vger.kernel.org # v4.13+ Cc: Francis Perron Reported-by: AutonomousCodeSecurity@microsoft.com Signed-off-by: Cen Zhang (Microsoft) Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 5b42140e12e8..9df06fdd894f 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -919,7 +919,7 @@ static void profile_query_cb(const struct aa_profile *profile, static ssize_t query_data(char *buf, size_t buf_len, char *query, size_t query_len) { - char *out; + char *out, *keydup; const char *key; struct label_it i; struct aa_label *label, *curr; @@ -941,11 +941,18 @@ static ssize_t query_data(char *buf, size_t buf_len, if (buf_len < sizeof(bytes) + sizeof(blocks)) return -EINVAL; /* not enough space */ + /* buf overlaps query, keep a copy of the key for the later lookups */ + keydup = kstrdup(key, GFP_KERNEL); + if (!keydup) + return -ENOMEM; + curr = begin_current_label_crit_section(&needput); label = aa_label_parse(curr, query, GFP_KERNEL, false, false); end_current_label_crit_section(curr, needput); - if (IS_ERR(label)) + if (IS_ERR(label)) { + kfree(keydup); return PTR_ERR(label); + } /* We are going to leave space for two numbers. The first is the total * number of bytes we are writing after the first number. This is so @@ -963,13 +970,14 @@ static ssize_t query_data(char *buf, size_t buf_len, if (!profile->data) continue; - data = rhashtable_lookup_fast(profile->data, &key, + data = rhashtable_lookup_fast(profile->data, &keydup, profile->data->p); if (data) { if (out + sizeof(outle32) + data->size > buf + buf_len) { aa_put_label(label); + kfree(keydup); return -EINVAL; /* not enough space */ } outle32 = __cpu_to_le32(data->size); @@ -981,6 +989,7 @@ static ssize_t query_data(char *buf, size_t buf_len, } } aa_put_label(label); + kfree(keydup); outle32 = __cpu_to_le32(out - buf - sizeof(bytes)); memcpy(buf, &outle32, sizeof(outle32)); From 6ec46f8f1977d218bf2494ce276152b2d01424af Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Wed, 14 Oct 2026 19:08:20 -0300 Subject: [PATCH 20/23] apparmor: remove fs entries created by a failed policy load aa_replace_profiles() creates the raw_data/ directory for the loaddata and the profile directories for new profiles before the last checks that can fail. When one of those checks fails, the fail path releases the ns lock without removing what this load created. The raw_data directory is named after the current ns revision, and the revision is only bumped by a successful load. The stale directory is only removed once the loaddata's last profile reference is dropped, from a workqueue. Until then, every load fails to create raw_data/ with -EEXIST. If the new profiles are never freed, which is the case when their dents are left behind pinning the proxy, that never happens and no policy can be loaded until reboot. While still holding the ns lock in the fail path, remove the profile directories of the new profiles and, if this load created it, the raw_data directory. Fixes: 5d5182cae401 ("apparmor: move to per loaddata files, instead of replicating in profiles") Cc: stable@vger.kernel.org Signed-off-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/policy.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 397f9ff78aeb..f6f7cdcb6212 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1191,6 +1191,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, struct aa_loaddata *rawdata_ent; const char *op; ssize_t count, error; + bool rawdata_created = false; LIST_HEAD(lh); op = mask & AA_MAY_REPLACE_POLICY ? OP_PROF_REPL : OP_PROF_LOAD; @@ -1330,6 +1331,7 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, ent = NULL; goto fail_lock; } + rawdata_created = true; } list_for_each_entry(ent, &lh, list) { if (!ent->old) { @@ -1425,6 +1427,18 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, return udata_sz; fail_lock: + /* + * Remove fs entries created by this load. Nothing else references + * them yet, and leaving them behind would leak the new profiles + * (their dents pin the proxy) and make the next load at the same, + * unbumped ns revision fail with -EEXIST on its raw_data dir. + */ + list_for_each_entry(tmp, &lh, list) { + if (!tmp->old) + __aafs_profile_rmdir(tmp->new); + } + if (rawdata_created) + __aa_fs_remove_rawdata(udata); mutex_unlock(&ns->lock); /* audit cause of failure */ From 97ba44a2aa4c7ab011a93ce94cb595200fe4ea4f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 29 Oct 2026 08:13:55 -0700 Subject: [PATCH 21/23] apparmor: on failed load ensure profile files are removed before parent The load interface does not guarantee the order profiles are loaded. When a parent is loaded after its child, and the load fails, the current code will issue a warning, and leak a dentry. For the loaded set of profiles, ensure that on failure list is ordered is traversed in reverse so any children processed after their parent is freed first. We do not need to deal with the cases where the children are processed before their parent, because either a parent outside the list already exists or a dummy parent outside of the list is created, and the list walk will not put those parents before their children. Fixes: 6ec46f8f1977d ("apparmor: remove fs entries created by a failed policy load") Signed-off-by: John Johansen --- security/apparmor/policy.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index f6f7cdcb6212..bfe969b86957 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1432,8 +1432,10 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, * them yet, and leaving them behind would leak the new profiles * (their dents pin the proxy) and make the next load at the same, * unbumped ns revision fail with -EEXIST on its raw_data dir. + * ensure children (which are at the end of the list are removed + * before their parent */ - list_for_each_entry(tmp, &lh, list) { + list_for_each_entry_reverse(tmp, &lh, list) { if (!tmp->old) __aafs_profile_rmdir(tmp->new); } From ce94927fa4173fcc1eca27bdc65a140064c8f1e6 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Wed, 14 Oct 2026 19:08:21 -0300 Subject: [PATCH 22/23] apparmor: fix profile leak when a policy load fails The error paths of aa_replace_profiles() and aa_unpack() do not break the refcount cycle between label and proxy. They just aa_put_profile() the new profiles, which drops the label refcount from 2 to 1, and the proxy keeps the profile alive. Every failed atomic policy load (e.g. "profile can not be replaced", or a later profile in the blob failing to unpack) leaks every new profile in the set, along with its proxy, rules and its loaddata reference. The leak is usually not visible from userspace, because the raw_data directory for a failed load is not created, but it becomes visible when the blob is deduplicated onto an existing loaddata, which then keeps its policy/raw_data/ directory forever. Add aa_put_unpublished_profile(), which drops the profile's own proxy before putting the profile, and use it on those error paths. The proxy of an unpacked profile is always freshly allocated and not referenced by anything else before the profile is published, so putting it is safe. Fixes: 637f688dc3dc ("apparmor: switch from profiles to using labels on contexts") Cc: stable@vger.kernel.org Signed-off-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/include/policy.h | 20 ++++++++++++++++++++ security/apparmor/policy.c | 3 +++ security/apparmor/policy_unpack.c | 4 +++- 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 0dcbf8cf1029..bb851654cb80 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -436,6 +436,26 @@ static inline void aa_put_profile(struct aa_profile *p) kref_put(&p->label.count.count, aa_label_kref); } +/** + * aa_put_unpublished_profile - put a profile that was never made visible + * @p: profile (MAYBE NULL) + * + * A newly allocated profile and its proxy reference each other + * (proxy->label holds a label refcount). That cycle is normally broken + * when the profile is replaced or removed, which redirects the proxy. + * A profile that is discarded before being added to the policy tree + * never goes through either path, so break the cycle here, otherwise + * the profile, its proxy and its rawdata are leaked. + */ +static inline void aa_put_unpublished_profile(struct aa_profile *p) +{ + if (p && p->label.proxy) { + aa_put_proxy(p->label.proxy); + p->label.proxy = NULL; + } + aa_put_profile(p); +} + static inline int AUDIT_MODE(const struct aa_profile *profile) { if (aa_g_audit != AUDIT_NORMAL) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index bfe969b86957..63f7821b4dff 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -1462,6 +1462,9 @@ ssize_t aa_replace_profiles(struct aa_ns *policy_ns, struct aa_label *label, } list_for_each_entry_safe(ent, tmp, &lh, list) { list_del_init(&ent->list); + /* new was never added to the policy tree */ + aa_put_unpublished_profile(ent->new); + ent->new = NULL; aa_load_ent_free(ent); } diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 7ebffb4ff74f..35dfa3b451f3 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1822,13 +1822,15 @@ int aa_unpack(struct aa_loaddata *udata, struct list_head *lh, fail_profile: kfree(ns_name); - aa_put_profile(profile); + aa_put_unpublished_profile(profile); fail: if (compressed_data) kvfree(compressed_data); list_for_each_entry_safe(ent, tmp, lh, list) { list_del_init(&ent->list); + aa_put_unpublished_profile(ent->new); + ent->new = NULL; aa_load_ent_free(ent); } From ed35bbe7d06e6505401eff3f3c7a36bd1f167a17 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 29 Oct 2026 16:53:12 -0700 Subject: [PATCH 23/23] apparmor: fix uninitialized variable warning in aa_inherit_files Fix the following warning All warnings (new ones prefixed by >>): >> security/apparmor/file.c:737:53: warning: variable 'file' is uninitialized when used here [-Wuninitialized] 737 | int error = aa_file_perm(OP_INHERIT, cred, label, file, | ^~~~ security/apparmor/file.c:724:36: note: initialize the variable 'file' to silence this warning 724 | struct file *devnull = NULL, *file; | ^ | = NULL 1 warning generated. vim +/file +737 security/apparmor/file.c 719 720 /* based on selinux's flush_unauthorized_files */ 721 void aa_inherit_files(const struct cred *cred, struct files_struct *files) 722 { 723 struct aa_label *label = aa_get_newest_cred_label(cred); 724 struct file *devnull = NULL, *file; 725 bool devnull_tried = false; 726 unsigned int n = 0; 727 728 revalidate_tty(cred, label); 729 730 /* 731 * Revalidate access to inherited open files. Each file is 732 * revalidated holding only a reference to it, with no locks 733 * held, as revalidation may sleep (e.g. unix socket mediation 734 * dropping path refs, or allocating path buffers). 735 */ 736 while ((n = iterate_fd(files, n, get_next_file, &file)) != 0) { > 737 int error = aa_file_perm(OP_INHERIT, cred, label, file, Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202610292346.ehqNujiE-lkp@intel.com/ Fixes: a913281f60015 ("apparmor: don't revalidate inherited files under files->file_lock") Signed-off-by: John Johansen --- security/apparmor/file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index b52781e05a5a..baca370ddfc0 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -721,7 +721,7 @@ static int get_next_file(const void *p, struct file *file, unsigned int fd) void aa_inherit_files(const struct cred *cred, struct files_struct *files) { struct aa_label *label = aa_get_newest_cred_label(cred); - struct file *devnull = NULL, *file; + struct file *devnull = NULL, *file = NULL; bool devnull_tried = false; unsigned int n = 0;