From 7db8c3c738118b6d9e5c3eb54e352fdae1bb0faf Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Sat, 22 Nov 2025 12:54:46 +0100 Subject: [PATCH 01/34] apparmor: replace sprintf with snprintf in aa_new_learning_profile Replace unbounded sprintf() calls with snprintf() to prevent potential buffer overflows in aa_new_learning_profile(). While the current code works correctly, snprintf() is safer and follows secure coding best practices. No functional changes. Signed-off-by: Thorsten Blum Signed-off-by: John Johansen --- security/apparmor/policy.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 50d5345ff5cb..b09323867fea 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -697,24 +697,27 @@ struct aa_profile *aa_new_learning_profile(struct aa_profile *parent, bool hat, struct aa_profile *p, *profile; const char *bname; char *name = NULL; + size_t name_sz; AA_BUG(!parent); if (base) { - name = kmalloc(strlen(parent->base.hname) + 8 + strlen(base), - gfp); + name_sz = strlen(parent->base.hname) + 8 + strlen(base); + name = kmalloc(name_sz, gfp); if (name) { - sprintf(name, "%s//null-%s", parent->base.hname, base); + snprintf(name, name_sz, "%s//null-%s", + parent->base.hname, base); goto name; } /* fall through to try shorter uniq */ } - name = kmalloc(strlen(parent->base.hname) + 2 + 7 + 8, gfp); + name_sz = strlen(parent->base.hname) + 2 + 7 + 8; + name = kmalloc(name_sz, gfp); if (!name) return NULL; - sprintf(name, "%s//null-%x", parent->base.hname, - atomic_inc_return(&parent->ns->uniq_null)); + snprintf(name, name_sz, "%s//null-%x", parent->base.hname, + atomic_inc_return(&parent->ns->uniq_null)); name: /* lookup to see if this is a dup creation */ From b31d3f7385fbb49681d44e7104cfa033cba4b1e8 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Sat, 22 Nov 2025 12:55:51 +0100 Subject: [PATCH 02/34] apparmor: Replace sprintf/strcpy with scnprintf/strscpy in aa_policy_init strcpy() is deprecated and sprintf() does not perform bounds checking either. Although an overflow is unlikely, it's better to proactively avoid it by using the safer strscpy() and scnprintf(), respectively. Additionally, unify memory allocation for 'hname' to simplify and improve aa_policy_init(). Closes: https://github.com/KSPP/linux/issues/88 Reviewed-by: Serge Hallyn Signed-off-by: Thorsten Blum Signed-off-by: John Johansen --- security/apparmor/lib.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 82dbb97ad406..acf7f5189bec 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -478,19 +478,17 @@ bool aa_policy_init(struct aa_policy *policy, const char *prefix, const char *name, gfp_t gfp) { char *hname; + size_t hname_sz; + hname_sz = (prefix ? strlen(prefix) + 2 : 0) + strlen(name) + 1; /* freed by policy_free */ - if (prefix) { - hname = aa_str_alloc(strlen(prefix) + strlen(name) + 3, gfp); - if (hname) - sprintf(hname, "%s//%s", prefix, name); - } else { - hname = aa_str_alloc(strlen(name) + 1, gfp); - if (hname) - strcpy(hname, name); - } + hname = aa_str_alloc(hname_sz, gfp); if (!hname) return false; + if (prefix) + scnprintf(hname, hname_sz, "%s//%s", prefix, name); + else + strscpy(hname, name, hname_sz); policy->hname = hname; /* base.name is a substring of fqname */ policy->name = basename(policy->hname); From 93d4dbdc8da0b8a3ba86f4a08868084f8da872e1 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Thu, 6 Nov 2025 15:51:38 +0100 Subject: [PATCH 03/34] apparmor: Replace deprecated strcpy in d_namespace_path strcpy() is deprecated; replace it with a direct '/' assignment. The buffer is already NUL-terminated, so there is no need to copy an additional NUL terminator as strcpy() did. Update the comment and add the local variable 'is_root' for clarity. Closes: https://github.com/KSPP/linux/issues/88 Signed-off-by: Thorsten Blum Signed-off-by: John Johansen --- security/apparmor/path.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/security/apparmor/path.c b/security/apparmor/path.c index d6c74c357ffd..65a0ca5cc1bd 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -164,12 +164,15 @@ static int d_namespace_path(const struct path *path, char *buf, char **name, } out: - /* - * Append "/" to the pathname. The root directory is a special - * case; it already ends in slash. + /* Append "/" to directory paths, except for root "/" which + * already ends in a slash. */ - if (!error && isdir && ((*name)[1] != '\0' || (*name)[0] != '/')) - strcpy(&buf[aa_g_path_max - 2], "/"); + if (!error && isdir) { + bool is_root = (*name)[0] == '/' && (*name)[1] == '\0'; + + if (!is_root) + buf[aa_g_path_max - 2] = '/'; + } return error; } From e2938ad00b21340c0362562dfedd7cfec0554d67 Mon Sep 17 00:00:00 2001 From: System Administrator Date: Thu, 9 Oct 2025 16:35:00 +0000 Subject: [PATCH 04/34] apparmor: fix NULL pointer dereference in __unix_needs_revalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When receiving file descriptors via SCM_RIGHTS, both the socket pointer and the socket's sk pointer can be NULL during socket setup or teardown, causing NULL pointer dereferences in __unix_needs_revalidation(). This is a regression in AppArmor 5.0.0 (kernel 6.17+) where the new __unix_needs_revalidation() function was added without proper NULL checks. The crash manifests as: BUG: kernel NULL pointer dereference, address: 0x0000000000000018 RIP: aa_file_perm+0xb7/0x3b0 (or +0xbe/0x3b0, +0xc0/0x3e0) Call Trace: apparmor_file_receive+0x42/0x80 security_file_receive+0x2e/0x50 receive_fd+0x1d/0xf0 scm_detach_fds+0xad/0x1c0 The function dereferences sock->sk->sk_family without checking if either sock or sock->sk is NULL first. Add NULL checks for both sock and sock->sk before accessing sk_family. Fixes: 88fec3526e841 ("apparmor: make sure unix socket labeling is correctly updated.") Reported-by: Jamin Mc Closes: https://bugzilla.proxmox.com/show_bug.cgi?id=7083 Closes: https://gitlab.com/apparmor/apparmor/-/issues/568 Signed-off-by: Fabian Grünbichler Signed-off-by: System Administrator Signed-off-by: John Johansen --- security/apparmor/file.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index c75820402878..919dbbbc87ab 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -578,6 +578,9 @@ static bool __unix_needs_revalidation(struct file *file, struct aa_label *label, return false; if (request & NET_PEER_MASK) return false; + /* sock and sock->sk can be NULL for sockets being set up or torn down */ + if (!sock || !sock->sk) + return false; if (sock->sk->sk_family == PF_UNIX) { struct aa_sk_ctx *ctx = aa_sock(sock->sk); From 00b67657535dfea56e84d11492f5c0f61d0af297 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 24 Nov 2025 15:07:42 -0800 Subject: [PATCH 05/34] apparmor: fix NULL sock in aa_sock_file_perm Deal with the potential that sock and sock-sk can be NULL during socket setup or teardown. This could lead to an oops. The fix for NULL pointer dereference in __unix_needs_revalidation shows this is at least possible for af_unix sockets. While the fix for af_unix sockets applies for newer mediation this is still the fall back path for older af_unix mediation and other sockets, so ensure it is covered. Fixes: 56974a6fcfef6 ("apparmor: add base infastructure for socket mediation") Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/net.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/net.c b/security/apparmor/net.c index 45cf25605c34..44c04102062f 100644 --- a/security/apparmor/net.c +++ b/security/apparmor/net.c @@ -326,8 +326,10 @@ int aa_sock_file_perm(const struct cred *subj_cred, struct aa_label *label, struct socket *sock = (struct socket *) file->private_data; AA_BUG(!label); - AA_BUG(!sock); - AA_BUG(!sock->sk); + + /* sock && sock->sk can be NULL for sockets being set up or torn down */ + if (!sock || !sock->sk) + return 0; if (sock->sk->sk_family == PF_UNIX) return aa_unix_file_perm(subj_cred, label, op, request, file); From 1c90ed1f14c9892c24d5252a2966470f4937f7a2 Mon Sep 17 00:00:00 2001 From: Thorsten Blum Date: Thu, 16 Oct 2025 17:41:10 +0200 Subject: [PATCH 06/34] apparmor: Replace deprecated strcpy with memcpy in gen_symlink_name strcpy() is deprecated; use memcpy() instead. Unlike strcpy(), memcpy() does not copy the NUL terminator from the source string, which would be overwritten anyway on every iteration when using strcpy(). snprintf() then ensures that 'char *s' is NUL-terminated. Replace the hard-coded path length to remove the magic number 6, and add a comment explaining the extra 11 bytes. Closes: https://github.com/KSPP/linux/issues/88 Signed-off-by: Thorsten Blum Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 907bd2667e28..91c1fcb78ac8 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -1607,16 +1607,20 @@ static char *gen_symlink_name(int depth, const char *dirname, const char *fname) { char *buffer, *s; int error; - int size = depth * 6 + strlen(dirname) + strlen(fname) + 11; + const char *path = "../../"; + size_t path_len = strlen(path); + int size; + /* Extra 11 bytes: "raw_data" (9) + two slashes "//" (2) */ + size = depth * path_len + strlen(dirname) + strlen(fname) + 11; s = buffer = kmalloc(size, GFP_KERNEL); if (!buffer) return ERR_PTR(-ENOMEM); for (; depth > 0; depth--) { - strcpy(s, "../../"); - s += 6; - size -= 6; + memcpy(s, path, path_len); + s += path_len; + size -= path_len; } error = snprintf(s, size, "raw_data/%s/%s", dirname, fname); From 64802f731214a51dfe3c6c27636b3ddafd003eb0 Mon Sep 17 00:00:00 2001 From: Helge Deller Date: Tue, 25 Nov 2025 16:11:07 +0100 Subject: [PATCH 07/34] AppArmor: Allow apparmor to handle unaligned dfa tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dfa tables can originate from kernel or userspace and 8-byte alignment isn't always guaranteed and as such may trigger unaligned memory accesses on various architectures. Resulting in the following [   73.901376] WARNING: CPU: 0 PID: 341 at security/apparmor/match.c:316 aa_dfa_unpack+0x6cc/0x720 [   74.015867] Modules linked in: binfmt_misc evdev flash sg drm drm_panel_orientation_quirks backlight i2c_core configfs nfnetlink autofs4 ext4 crc16 mbcache jbd2 hid_generic usbhid sr_mod hid cdrom sd_mod ata_generic ohci_pci ehci_pci ehci_hcd ohci_hcd pata_ali libata sym53c8xx scsi_transport_spi tg3 scsi_mod usbcore libphy scsi_common mdio_bus usb_common [   74.428977] CPU: 0 UID: 0 PID: 341 Comm: apparmor_parser Not tainted 6.18.0-rc6+ #9 NONE [   74.536543] Call Trace: [   74.568561] [<0000000000434c24>] dump_stack+0x8/0x18 [   74.633757] [<0000000000476438>] __warn+0xd8/0x100 [   74.696664] [<00000000004296d4>] warn_slowpath_fmt+0x34/0x74 [   74.771006] [<00000000008db28c>] aa_dfa_unpack+0x6cc/0x720 [   74.843062] [<00000000008e643c>] unpack_pdb+0xbc/0x7e0 [   74.910545] [<00000000008e7740>] unpack_profile+0xbe0/0x1300 [   74.984888] [<00000000008e82e0>] aa_unpack+0xe0/0x6a0 [   75.051226] [<00000000008e3ec4>] aa_replace_profiles+0x64/0x1160 [   75.130144] [<00000000008d4d90>] policy_update+0xf0/0x280 [   75.201057] [<00000000008d4fc8>] profile_replace+0xa8/0x100 [   75.274258] [<0000000000766bd0>] vfs_write+0x90/0x420 [   75.340594] [<00000000007670cc>] ksys_write+0x4c/0xe0 [   75.406932] [<0000000000767174>] sys_write+0x14/0x40 [   75.472126] [<0000000000406174>] linux_sparc_syscall+0x34/0x44 [   75.548802] ---[ end trace 0000000000000000 ]--- [   75.609503] dfa blob stream 0xfff0000008926b96 not aligned. [   75.682695] Kernel unaligned access at TPC[8db2a8] aa_dfa_unpack+0x6e8/0x720 Work around it by using the get_unaligned_xx() helpers. Fixes: e6e8bf418850d ("apparmor: fix restricted endian type warnings for dfa unpack") Reported-by: John Paul Adrian Glaubitz Closes: https://github.com/sparclinux/issues/issues/30 Signed-off-by: Helge Deller Signed-off-by: John Johansen --- security/apparmor/match.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/security/apparmor/match.c b/security/apparmor/match.c index c5a91600842a..26e82ba879d4 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -15,6 +15,7 @@ #include #include #include +#include #include "include/lib.h" #include "include/match.h" @@ -42,11 +43,11 @@ static struct table_header *unpack_table(char *blob, size_t bsize) /* loaded td_id's start at 1, subtract 1 now to avoid doing * it every time we use td_id as an index */ - th.td_id = be16_to_cpu(*(__be16 *) (blob)) - 1; + th.td_id = get_unaligned_be16(blob) - 1; if (th.td_id > YYTD_ID_MAX) goto out; - th.td_flags = be16_to_cpu(*(__be16 *) (blob + 2)); - th.td_lolen = be32_to_cpu(*(__be32 *) (blob + 8)); + th.td_flags = get_unaligned_be16(blob + 2); + th.td_lolen = get_unaligned_be32(blob + 8); blob += sizeof(struct table_header); if (!(th.td_flags == YYTD_DATA16 || th.td_flags == YYTD_DATA32 || @@ -313,14 +314,14 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags) if (size < sizeof(struct table_set_header)) goto fail; - if (ntohl(*(__be32 *) data) != YYTH_MAGIC) + if (get_unaligned_be32(data) != YYTH_MAGIC) goto fail; - hsize = ntohl(*(__be32 *) (data + 4)); + hsize = get_unaligned_be32(data + 4); if (size < hsize) goto fail; - dfa->flags = ntohs(*(__be16 *) (data + 12)); + dfa->flags = get_unaligned_be16(data + 12); if (dfa->flags & ~(YYTH_FLAGS)) goto fail; @@ -329,7 +330,7 @@ struct aa_dfa *aa_dfa_unpack(void *blob, size_t size, int flags) * if (dfa->flags & YYTH_FLAGS_OOB_TRANS) { * if (hsize < 16 + 4) * goto fail; - * dfa->max_oob = ntol(*(__be32 *) (data + 16)); + * dfa->max_oob = get_unaligned_be32(data + 16); * if (dfa->max <= MAX_OOB_SUPPORTED) { * pr_err("AppArmor DFA OOB greater than supported\n"); * goto fail; From 6fc367bfd4c8886e6b1742aabbd1c0bdc310db3a Mon Sep 17 00:00:00 2001 From: Helge Deller Date: Wed, 26 Nov 2025 21:15:04 +0100 Subject: [PATCH 08/34] apparmor: Fix & Optimize table creation from possibly unaligned memory Source blob may come from userspace and might be unaligned. Try to optize the copying process by avoiding unaligned memory accesses. - Added Fixes tag - Added "Fix &" to description as this doesn't just optimize but fixes a potential unaligned memory access Fixes: e6e8bf418850d ("apparmor: fix restricted endian type warnings for dfa unpack") Signed-off-by: Helge Deller [jj: remove duplicate word "convert" in comment trigger checkpatch warning] Signed-off-by: John Johansen --- security/apparmor/include/match.h | 12 +++++++----- security/apparmor/match.c | 7 +++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/security/apparmor/include/match.h b/security/apparmor/include/match.h index 1fbe82f5021b..0dde8eda3d1a 100644 --- a/security/apparmor/include/match.h +++ b/security/apparmor/include/match.h @@ -104,16 +104,18 @@ struct aa_dfa { struct table_header *tables[YYTD_ID_TSIZE]; }; -#define byte_to_byte(X) (X) - #define UNPACK_ARRAY(TABLE, BLOB, LEN, TTYPE, BTYPE, NTOHX) \ do { \ typeof(LEN) __i; \ TTYPE *__t = (TTYPE *) TABLE; \ BTYPE *__b = (BTYPE *) BLOB; \ - for (__i = 0; __i < LEN; __i++) { \ - __t[__i] = NTOHX(__b[__i]); \ - } \ + BUILD_BUG_ON(sizeof(TTYPE) != sizeof(BTYPE)); \ + if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) \ + memcpy(__t, __b, (LEN) * sizeof(BTYPE)); \ + else /* copy & convert from big-endian */ \ + for (__i = 0; __i < LEN; __i++) { \ + __t[__i] = NTOHX(&__b[__i]); \ + } \ } while (0) static inline size_t table_size(size_t len, size_t el_size) diff --git a/security/apparmor/match.c b/security/apparmor/match.c index 26e82ba879d4..bbeb3be68572 100644 --- a/security/apparmor/match.c +++ b/security/apparmor/match.c @@ -67,14 +67,13 @@ static struct table_header *unpack_table(char *blob, size_t bsize) table->td_flags = th.td_flags; table->td_lolen = th.td_lolen; if (th.td_flags == YYTD_DATA8) - UNPACK_ARRAY(table->td_data, blob, th.td_lolen, - u8, u8, byte_to_byte); + memcpy(table->td_data, blob, th.td_lolen); else if (th.td_flags == YYTD_DATA16) UNPACK_ARRAY(table->td_data, blob, th.td_lolen, - u16, __be16, be16_to_cpu); + u16, __be16, get_unaligned_be16); else if (th.td_flags == YYTD_DATA32) UNPACK_ARRAY(table->td_data, blob, th.td_lolen, - u32, __be32, be32_to_cpu); + u32, __be32, get_unaligned_be32); else goto fail; /* if table was vmalloced make sure the page tables are synced From c140dcd1246bfe705921ca881bbb247ff1ba2bca Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 1 Aug 2025 02:21:44 -0700 Subject: [PATCH 09/34] apparmor: make str table more generic and be able to have multiple entries The strtable is currently limited to a single entry string on unpack even though domain has the concept of multiple entries within it. Make this a reality as it will be used for tags and more advanced domain transitions. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/domain.c | 2 +- security/apparmor/include/lib.h | 13 +++- security/apparmor/lib.c | 11 +-- security/apparmor/policy.c | 2 +- security/apparmor/policy_unpack.c | 110 ++++++++++++++++++++---------- 5 files changed, 91 insertions(+), 47 deletions(-) diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index 267da82afb14..ee2df25f355d 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -529,7 +529,7 @@ struct aa_label *x_table_lookup(struct aa_profile *profile, u32 xindex, /* TODO: move lookup parsing to unpack time so this is a straight * index into the resultant label */ - for (next = rules->file->trans.table[index]; next; + for (next = rules->file->trans.table[index].strs; next; next = next_name(xtype, next)) { const char *lookup = (*next == '&') ? next + 1 : next; *name = next; diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h index 444197075fd6..194be85e7fff 100644 --- a/security/apparmor/include/lib.h +++ b/security/apparmor/include/lib.h @@ -30,6 +30,7 @@ extern struct aa_dfa *stacksplitdfa; #define DEBUG_DOMAIN 4 #define DEBUG_POLICY 8 #define DEBUG_INTERFACE 0x10 +#define DEBUG_UNPACK 0x40 #define DEBUG_ALL 0x1f /* update if new DEBUG_X added */ #define DEBUG_PARSE_ERROR (-1) @@ -119,13 +120,19 @@ static inline bool path_mediated_fs(struct dentry *dentry) return !(dentry->d_sb->s_flags & SB_NOUSER); } -struct aa_str_table { +struct aa_str_table_ent { + int count; int size; - char **table; + char *strs; +}; + +struct aa_str_table { + int size; + struct aa_str_table_ent *table; }; -void aa_free_str_table(struct aa_str_table *table); bool aa_resize_str_table(struct aa_str_table *t, int newsize, gfp_t gfp); +void aa_destroy_str_table(struct aa_str_table *table); struct counted_str { struct kref count; diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index acf7f5189bec..7ef1b9ba7fb6 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -44,6 +44,7 @@ static struct val_table_ent debug_values_table[] = { { "domain", DEBUG_DOMAIN }, { "policy", DEBUG_POLICY }, { "interface", DEBUG_INTERFACE }, + { "unpack", DEBUG_UNPACK }, { NULL, 0 } }; @@ -118,7 +119,7 @@ int aa_print_debug_params(char *buffer) bool aa_resize_str_table(struct aa_str_table *t, int newsize, gfp_t gfp) { - char **n; + struct aa_str_table_ent *n; int i; if (t->size == newsize) @@ -129,7 +130,7 @@ bool aa_resize_str_table(struct aa_str_table *t, int newsize, gfp_t gfp) for (i = 0; i < min(t->size, newsize); i++) n[i] = t->table[i]; for (; i < t->size; i++) - kfree_sensitive(t->table[i]); + kfree_sensitive(t->table[i].strs); if (newsize > t->size) memset(&n[t->size], 0, (newsize-t->size)*sizeof(*n)); kfree_sensitive(t->table); @@ -140,10 +141,10 @@ bool aa_resize_str_table(struct aa_str_table *t, int newsize, gfp_t gfp) } /** - * aa_free_str_table - free entries str table + * aa_destroy_str_table - free entries str table * @t: the string table to free (MAYBE NULL) */ -void aa_free_str_table(struct aa_str_table *t) +void aa_destroy_str_table(struct aa_str_table *t) { int i; @@ -152,7 +153,7 @@ void aa_free_str_table(struct aa_str_table *t) return; for (i = 0; i < t->size; i++) - kfree_sensitive(t->table[i]); + kfree_sensitive(t->table[i].strs); kfree_sensitive(t->table); t->table = NULL; t->size = 0; diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index b09323867fea..2a8a0e4a19fc 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -104,7 +104,7 @@ static void aa_free_pdb(struct aa_policydb *pdb) if (pdb) { aa_put_dfa(pdb->dfa); kvfree(pdb->perms); - aa_free_str_table(&pdb->trans); + aa_destroy_str_table(&pdb->trans); kfree(pdb); } } diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 7523971e37d9..4f7cb42073e4 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -450,20 +450,72 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e, int flags) return dfa; } +static int process_strs_entry(char *str, int size, bool multi) +{ + int c = 1; + + if (size <= 0) + return -1; + if (multi) { + if (size < 2) + return -2; + /* multi ends with double \0 */ + if (str[size - 2]) + return -3; + } + + char *save = str; + char *pos = str; + char *end = multi ? str + size - 2 : str + size - 1; + /* count # of internal \0 */ + while (str < end) { + if (str == pos) { + /* starts with ... */ + if (!*str) { + AA_DEBUG(DEBUG_UNPACK, + "starting with null save=%lu size %d c=%d", + str - save, size, c); + return -4; + } + if (isspace(*str)) + return -5; + if (*str == ':') { + /* :ns_str\0str\0 + * first character after : must be valid + */ + if (!str[1]) + return -6; + } + } else if (!*str) { + if (*pos == ':') + *str = ':'; + else + c++; + pos = str + 1; + } + str++; + } /* while */ + + return c; +} + /** - * unpack_trans_table - unpack a profile transition table + * unpack_strs_table - unpack a profile transition table * @e: serialized data extent information (NOT NULL) + * @name: name of table (MAY BE NULL) + * @multi: allow multiple strings on a single entry * @strs: str table to unpack to (NOT NULL) * * Returns: true if table successfully unpacked or not present */ -static bool unpack_trans_table(struct aa_ext *e, struct aa_str_table *strs) +static bool unpack_strs_table(struct aa_ext *e, const char *name, bool multi, + struct aa_str_table *strs) { void *saved_pos = e->pos; - char **table = NULL; + struct aa_str_table_ent *table = NULL; /* exec table is optional */ - if (aa_unpack_nameX(e, AA_STRUCT, "xtable")) { + if (aa_unpack_nameX(e, AA_STRUCT, name)) { u16 size; int i; @@ -475,7 +527,8 @@ static bool unpack_trans_table(struct aa_ext *e, struct aa_str_table *strs) * for size check here */ goto fail; - table = kcalloc(size, sizeof(char *), GFP_KERNEL); + table = kcalloc(size, sizeof(struct aa_str_table_ent), + GFP_KERNEL); if (!table) goto fail; @@ -483,41 +536,23 @@ static bool unpack_trans_table(struct aa_ext *e, struct aa_str_table *strs) strs->size = size; for (i = 0; i < size; i++) { char *str; - int c, j, pos, size2 = aa_unpack_strdup(e, &str, NULL); + int c, size2 = aa_unpack_strdup(e, &str, NULL); /* aa_unpack_strdup verifies that the last character is * null termination byte. */ - if (!size2) + c = process_strs_entry(str, size2, multi); + if (c <= 0) { + AA_DEBUG(DEBUG_UNPACK, "process_strs %d", c); goto fail; - table[i] = str; - /* verify that name doesn't start with space */ - if (isspace(*str)) - goto fail; - - /* count internal # of internal \0 */ - for (c = j = 0; j < size2 - 1; j++) { - if (!str[j]) { - pos = j; - c++; - } } - if (*str == ':') { - /* first character after : must be valid */ - if (!str[1]) - goto fail; - /* beginning with : requires an embedded \0, - * verify that exactly 1 internal \0 exists - * trailing \0 already verified by aa_unpack_strdup - * - * convert \0 back to : for label_parse - */ - if (c == 1) - str[pos] = ':'; - else if (c > 1) - goto fail; - } else if (c) + if (!multi && c > 1) { + AA_DEBUG(DEBUG_UNPACK, "!multi && c > 1"); /* 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; @@ -527,7 +562,7 @@ static bool unpack_trans_table(struct aa_ext *e, struct aa_str_table *strs) return true; fail: - aa_free_str_table(strs); + aa_destroy_str_table(strs); e->pos = saved_pos; return false; } @@ -795,13 +830,14 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, * transition table may be present even when the dfa is * not. For compatibility reasons unpack and discard. */ - if (!unpack_trans_table(e, &pdb->trans) && required_trans) { + if (!unpack_strs_table(e, "xtable", false, &pdb->trans) && + required_trans) { *info = "failed to unpack profile transition table"; goto fail; } if (!pdb->dfa && pdb->trans.table) - aa_free_str_table(&pdb->trans); + aa_destroy_str_table(&pdb->trans); /* TODO: * - move compat mapping here, requires dfa merging first @@ -1268,7 +1304,7 @@ static bool verify_perms(struct aa_policydb *pdb) } /* deal with incorrectly constructed string tables */ if (xmax == -1) { - aa_free_str_table(&pdb->trans); + aa_destroy_str_table(&pdb->trans); } else if (pdb->trans.size > xmax + 1) { if (!aa_resize_str_table(&pdb->trans, xmax + 1, GFP_KERNEL)) return false; From 3d28e2397af7a89ac3de33c686ed404cda59b5d5 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 1 Apr 2025 15:51:41 -0700 Subject: [PATCH 10/34] apparmor: add support loading per permission tagging Add support for the per permission tag index for a given permission set. This will be used by both meta-data tagging, to allow annotating accept states with context and debug information. As well as by rule tainting and triggers to specify the taint or trigger to be applied. Since these are low frequency ancillary data items they are stored in a tighter packed format to that allows for sharing and reuse of the strings between permissions and accept states. Reducing the amount of kernel memory use at the cost of having to go through a couple if index based indirections. The tags are just strings that has no meaning with out context. When used as meta-data for auditing and debugging its entirely information for userspace, but triggers, and tainting can be used to affect the domain. However they all exist in the same packed data set and can be shared between different uses. Signed-off-by: John Johansen --- security/apparmor/file.c | 1 + security/apparmor/include/audit.h | 2 + security/apparmor/include/lib.h | 5 +- security/apparmor/include/policy.h | 32 +++- security/apparmor/lib.c | 2 + security/apparmor/policy.c | 8 + security/apparmor/policy_compat.c | 10 +- security/apparmor/policy_unpack.c | 227 +++++++++++++++++++++++++++-- 8 files changed, 269 insertions(+), 18 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index 919dbbbc87ab..b69fece45ade 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -103,6 +103,7 @@ int aa_audit_file(const struct cred *subj_cred, ad.subj_cred = subj_cred; ad.request = request; + ad.tags = perms->tag; ad.name = name; ad.fs.target = target; ad.peer = tlabel; diff --git a/security/apparmor/include/audit.h b/security/apparmor/include/audit.h index 1a71a94ea19c..aa00b34404f9 100644 --- a/security/apparmor/include/audit.h +++ b/security/apparmor/include/audit.h @@ -119,6 +119,8 @@ struct apparmor_audit_data { const char *info; u32 request; u32 denied; + u32 tags; + union { /* these entries require a custom callback fn */ struct { diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h index 194be85e7fff..7ca8a92c449c 100644 --- a/security/apparmor/include/lib.h +++ b/security/apparmor/include/lib.h @@ -30,9 +30,10 @@ extern struct aa_dfa *stacksplitdfa; #define DEBUG_DOMAIN 4 #define DEBUG_POLICY 8 #define DEBUG_INTERFACE 0x10 -#define DEBUG_UNPACK 0x40 +#define DEBUG_UNPACK 0x20 +#define DEBUG_TAGS 0x40 -#define DEBUG_ALL 0x1f /* update if new DEBUG_X added */ +#define DEBUG_ALL 0x7f /* update if new DEBUG_X added */ #define DEBUG_PARSE_ERROR (-1) #define DEBUG_ON (aa_g_debug != DEBUG_NONE) diff --git a/security/apparmor/include/policy.h b/security/apparmor/include/policy.h index 4c50875c9d13..5115ebae2661 100644 --- a/security/apparmor/include/policy.h +++ b/security/apparmor/include/policy.h @@ -79,11 +79,33 @@ enum profile_mode { }; +struct aa_tags_header { + u32 mask; /* bit mask matching permissions */ + u32 count; /* number of strings per entry */ + u32 size; /* size of all strings covered by count */ + u32 tags; /* index into string table */ +}; + +struct aa_tags_struct { + struct { + u32 size; /* number of entries in tagsets */ + u32 *table; /* indexes into headers & strs */ + } sets; + struct { + u32 size; /* number of headers == num of strs */ + struct aa_tags_header *table; + } hdrs; + struct aa_str_table strs; +}; + /* struct aa_policydb - match engine for a policy - * count: refcount for the pdb - * dfa: dfa pattern match - * perms: table of permissions - * strs: table of strings, index by x + * @count: refcount for the pdb + * @dfa: dfa pattern match + * @perms: table of permissions + * @size: number of entries in @perms + * @trans: table of strings, index by x + * @tags: table of tags that perms->tag indexes + * @start:_states to start in for each class * start: set of start states for the different classes of data */ struct aa_policydb { @@ -94,11 +116,13 @@ struct aa_policydb { u32 size; }; struct aa_str_table trans; + struct aa_tags_struct tags; aa_state_t start[AA_CLASS_LAST + 1]; }; extern struct aa_policydb *nullpdb; +void aa_destroy_tags(struct aa_tags_struct *tags); struct aa_policydb *aa_alloc_pdb(gfp_t gfp); void aa_pdb_free_kref(struct kref *kref); diff --git a/security/apparmor/lib.c b/security/apparmor/lib.c index 7ef1b9ba7fb6..d0b82771df01 100644 --- a/security/apparmor/lib.c +++ b/security/apparmor/lib.c @@ -45,6 +45,7 @@ static struct val_table_ent debug_values_table[] = { { "policy", DEBUG_POLICY }, { "interface", DEBUG_INTERFACE }, { "unpack", DEBUG_UNPACK }, + { "tags", DEBUG_TAGS }, { NULL, 0 } }; @@ -511,3 +512,4 @@ void aa_policy_destroy(struct aa_policy *policy) /* don't free name as its a subset of hname */ aa_put_str(policy->hname); } + diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 2a8a0e4a19fc..f2cef22ed729 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -98,6 +98,13 @@ const char *const aa_profile_mode_names[] = { "user", }; +void aa_destroy_tags(struct aa_tags_struct *tags) +{ + kfree_sensitive(tags->hdrs.table); + kfree_sensitive(tags->sets.table); + aa_destroy_str_table(&tags->strs); + memset(tags, 0, sizeof(*tags)); +} static void aa_free_pdb(struct aa_policydb *pdb) { @@ -105,6 +112,7 @@ static void aa_free_pdb(struct aa_policydb *pdb) aa_put_dfa(pdb->dfa); kvfree(pdb->perms); aa_destroy_str_table(&pdb->trans); + aa_destroy_tags(&pdb->tags); kfree(pdb); } } diff --git a/security/apparmor/policy_compat.c b/security/apparmor/policy_compat.c index cfc2207e5a12..c863fc10a6f7 100644 --- a/security/apparmor/policy_compat.c +++ b/security/apparmor/policy_compat.c @@ -263,9 +263,15 @@ static struct aa_perms *compute_perms(struct aa_dfa *dfa, u32 version, *size = state_count; /* zero init so skip the trap state (state == 0) */ - for (state = 1; state < state_count; state++) + for (state = 1; state < state_count; state++) { table[state] = compute_perms_entry(dfa, state, version); - + AA_DEBUG(DEBUG_UNPACK, + "[%d]: (0x%x/0x%x/0x%x//0x%x/0x%x//0x%x), converted from accept1: 0x%x, accept2: 0x%x", + state, table[state].allow, table[state].deny, + table[state].prompt, table[state].audit, + table[state].quiet, table[state].xindex, + ACCEPT_TABLE(dfa)[state], ACCEPT_TABLE2(dfa)[state]); + } return table; } diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 4f7cb42073e4..b6e18ddff331 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -506,13 +506,14 @@ static int process_strs_entry(char *str, int size, bool multi) * @multi: allow multiple strings on a single entry * @strs: str table to unpack to (NOT NULL) * - * Returns: true if table successfully unpacked or not present + * Returns: 0 if table successfully unpacked or not present, else error */ -static bool unpack_strs_table(struct aa_ext *e, const char *name, bool multi, +static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, struct aa_str_table *strs) { void *saved_pos = e->pos; struct aa_str_table_ent *table = NULL; + int error = -EPROTO; /* exec table is optional */ if (aa_unpack_nameX(e, AA_STRUCT, name)) { @@ -529,9 +530,10 @@ static bool unpack_strs_table(struct aa_ext *e, const char *name, bool multi, goto fail; table = kcalloc(size, sizeof(struct aa_str_table_ent), GFP_KERNEL); - if (!table) + if (!table) { + error = -ENOMEM; goto fail; - + } strs->table = table; strs->size = size; for (i = 0; i < size; i++) { @@ -542,7 +544,8 @@ static bool unpack_strs_table(struct aa_ext *e, const char *name, bool multi, */ c = process_strs_entry(str, size2, multi); if (c <= 0) { - AA_DEBUG(DEBUG_UNPACK, "process_strs %d", c); + AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", + c, i, e->pos - saved_pos); goto fail; } if (!multi && c > 1) { @@ -559,12 +562,12 @@ static bool unpack_strs_table(struct aa_ext *e, const char *name, bool multi, if (!aa_unpack_nameX(e, AA_STRUCTEND, NULL)) goto fail; } - return true; + return 0; fail: aa_destroy_str_table(strs); e->pos = saved_pos; - return false; + return error; } static bool unpack_xattrs(struct aa_ext *e, struct aa_profile *profile) @@ -679,6 +682,204 @@ fail: return false; } + +static bool verify_tags(struct aa_tags_struct *tags, const char **info) +{ + if ((tags->hdrs.size && !tags->hdrs.table) || + (!tags->hdrs.size && tags->hdrs.table)) { + *info = "failed verification tag.hdrs disagree"; + return false; + } + if ((tags->sets.size && !tags->sets.table) || + (!tags->sets.size && tags->sets.table)) { + *info = "failed verification tag.sets disagree"; + return false; + } + if ((tags->strs.size && !tags->strs.table) || + (!tags->strs.size && tags->strs.table)) { + *info = "failed verification tags->strs disagree"; + return false; + } + /* no data present */ + if (!tags->sets.size && !tags->hdrs.size && !tags->strs.size) { + return true; + } else if (!(tags->sets.size && tags->hdrs.size && tags->strs.size)) { + /* some data present but not all */ + *info = "failed verification tags partial data present"; + return false; + } + + u32 i; + + for (i = 0; i < tags->sets.size; i++) { + /* count followed by count indexes into hdrs */ + u32 cnt = tags->sets.table[i]; + + if (i+cnt >= tags->sets.size) { + AA_DEBUG(DEBUG_UNPACK, + "tagset too large %d+%d > sets.table[%d]", + i, cnt, tags->sets.size); + *info = "failed verification tagset too large"; + return false; + } + for (; cnt; cnt--) { + if (tags->sets.table[++i] >= tags->hdrs.size) { + AA_DEBUG(DEBUG_UNPACK, + "tagsets idx out of bounds cnt %d sets.table[%d] >= %d", + cnt, i-1, tags->hdrs.size); + *info = "failed verification tagsets idx out of bounds"; + return false; + } + } + } + for (i = 0; i < tags->hdrs.size; i++) { + u32 idx = tags->hdrs.table[i].tags; + + if (idx >= tags->strs.size) { + AA_DEBUG(DEBUG_UNPACK, + "tag.hdrs idx oob idx %d > tags->strs.size=%d", + idx, tags->strs.size); + *info = "failed verification tags.hdrs idx out of bounds"; + return false; + } + if (tags->hdrs.table[i].count != tags->strs.table[idx].count) { + AA_DEBUG(DEBUG_UNPACK, "hdrs.table[%d].count=%d != tags->strs.table[%d]=%d", + i, tags->hdrs.table[i].count, idx, tags->strs.table[idx].count); + *info = "failed verification tagd.hdrs[idx].count"; + return false; + } + if (tags->hdrs.table[i].size != tags->strs.table[idx].size) { + AA_DEBUG(DEBUG_UNPACK, "hdrs.table[%d].size=%d != strs.table[%d].size=%d", + i, tags->hdrs.table[i].size, idx, tags->strs.table[idx].size); + *info = "failed verification tagd.hdrs[idx].size"; + return false; + } + } + + return true; +} + +static int unpack_tagsets(struct aa_ext *e, struct aa_tags_struct *tags) +{ + u32 *sets; + u16 i, size; + int error = -EPROTO; + void *pos = e->pos; + + if (!aa_unpack_array(e, "sets", &size)) + goto fail_reset; + sets = kcalloc(size, sizeof(u32), GFP_KERNEL); + if (!sets) { + error = -ENOMEM; + goto fail_reset; + } + for (i = 0; i < size; i++) { + if (!aa_unpack_u32(e, &sets[i], NULL)) + goto fail; + } + if (!aa_unpack_nameX(e, AA_ARRAYEND, NULL)) + goto fail; + + tags->sets.size = size; + tags->sets.table = sets; + + return 0; + +fail: + kfree_sensitive(sets); +fail_reset: + e->pos = pos; + return error; +} + +static bool unpack_tag_header_ent(struct aa_ext *e, struct aa_tags_header *h) +{ + return aa_unpack_u32(e, &h->mask, NULL) && + aa_unpack_u32(e, &h->count, NULL) && + aa_unpack_u32(e, &h->size, NULL) && + aa_unpack_u32(e, &h->tags, NULL); +} + +static int unpack_tag_headers(struct aa_ext *e, struct aa_tags_struct *tags) +{ + struct aa_tags_header *hdrs; + u16 i, size; + int error = -EPROTO; + void *pos = e->pos; + + if (!aa_unpack_array(e, "hdrs", &size)) + goto fail_reset; + hdrs = kcalloc(size, sizeof(struct aa_tags_header), GFP_KERNEL); + if (!hdrs) { + error = -ENOMEM; + goto fail_reset; + } + for (i = 0; i < size; i++) { + if (!unpack_tag_header_ent(e, &hdrs[i])) + goto fail; + } + if (!aa_unpack_nameX(e, AA_ARRAYEND, NULL)) + goto fail; + + tags->hdrs.size = size; + tags->hdrs.table = hdrs; + AA_DEBUG(DEBUG_UNPACK, "headers %ld size %d", (long) hdrs, size); + return true; + +fail: + kfree_sensitive(hdrs); +fail_reset: + e->pos = pos; + return error; +} + + +static size_t unpack_tags(struct aa_ext *e, struct aa_tags_struct *tags, + const char **info) +{ + int error = -EPROTO; + void *pos = e->pos; + + AA_BUG(!tags); + /* policy tags are optional */ + if (aa_unpack_nameX(e, AA_STRUCT, "tags")) { + u32 version; + + if (!aa_unpack_u32(e, &version, "version") || version != 1) { + *info = "invalid tags version"; + goto fail_reset; + } + error = unpack_strs_table(e, "strs", true, &tags->strs); + if (error) { + *info = "failed to unpack profile tag.strs"; + goto fail; + } + error = unpack_tag_headers(e, tags); + if (error) { + *info = "failed to unpack profile tag.headers"; + goto fail; + } + error = unpack_tagsets(e, tags); + if (error) { + *info = "failed to unpack profile tag.sets"; + goto fail; + } + if (!aa_unpack_nameX(e, AA_STRUCTEND, NULL)) + goto fail; + + if (!verify_tags(tags, info)) + goto fail; + } + + return 0; + +fail: + aa_destroy_tags(tags); +fail_reset: + e->pos = pos; + return error; +} + static bool unpack_perm(struct aa_ext *e, u32 version, struct aa_perms *perm) { u32 reserved; @@ -758,6 +959,11 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, if (!pdb) return -ENOMEM; + AA_DEBUG(DEBUG_UNPACK, "unpacking tags"); + if (unpack_tags(e, &pdb->tags, info) < 0) + goto fail; + AA_DEBUG(DEBUG_UNPACK, "done unpacking tags"); + size = unpack_perms_table(e, &pdb->perms); if (size < 0) { error = size; @@ -830,8 +1036,8 @@ static int unpack_pdb(struct aa_ext *e, struct aa_policydb **policy, * transition table may be present even when the dfa is * not. For compatibility reasons unpack and discard. */ - if (!unpack_strs_table(e, "xtable", false, &pdb->trans) && - required_trans) { + error = unpack_strs_table(e, "xtable", false, &pdb->trans); + if (error && required_trans) { *info = "failed to unpack profile transition table"; goto fail; } @@ -1094,6 +1300,7 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) goto fail; } else if (rules->file->dfa) { if (!rules->file->perms) { + AA_DEBUG(DEBUG_UNPACK, "compat mapping perms"); error = aa_compat_map_file(rules->file); if (error) { info = "failed to remap file permission table"; @@ -1296,7 +1503,7 @@ static bool verify_perms(struct aa_policydb *pdb) if (xmax < xidx) xmax = xidx; } - if (pdb->perms[i].tag && pdb->perms[i].tag >= pdb->trans.size) + if (pdb->perms[i].tag && pdb->perms[i].tag >= pdb->tags.sets.size) return false; if (pdb->perms[i].label && pdb->perms[i].label >= pdb->trans.size) From 8d34e16f7f2b51f880957f2caadaae731ee28867 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maxime=20B=C3=A9lair?= Date: Mon, 21 Jul 2025 16:46:44 +0200 Subject: [PATCH 11/34] apparmor: userns: Add support for execpath in userns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This new field allows reliable identification of the binary that triggered a denial since the existing field (comm) only gives the name of the binary, not its path. Thus comm doesn't work for binaries outside of $PATH or works unreliably when two binaries have the same name. Additionally comm can be modified by a program, for example, comm="(tor)" or comm=4143504920506F6C6C6572 (= ACPI Poller). Signed-off-by: Maxime Bélair Signed-off-by: John Johansen --- security/apparmor/task.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/security/apparmor/task.c b/security/apparmor/task.c index c9bc9cc69475..0db0e81b4600 100644 --- a/security/apparmor/task.c +++ b/security/apparmor/task.c @@ -15,6 +15,7 @@ #include #include +#include "include/path.h" #include "include/audit.h" #include "include/cred.h" #include "include/policy.h" @@ -300,16 +301,47 @@ int aa_may_ptrace(const struct cred *tracer_cred, struct aa_label *tracer, xrequest, &sa)); } +static const char *get_current_exe_path(char *buffer, int buffer_size) +{ + struct file *exe_file; + struct path p; + const char *path_str; + + exe_file = get_task_exe_file(current); + if (!exe_file) + return ERR_PTR(-ENOENT); + p = exe_file->f_path; + path_get(&p); + + if (aa_path_name(&p, FLAG_VIEW_SUBNS, buffer, &path_str, NULL, NULL)) + return ERR_PTR(-ENOMEM); + + fput(exe_file); + path_put(&p); + + return path_str; +} + /* call back to audit ptrace fields */ static void audit_ns_cb(struct audit_buffer *ab, void *va) { struct apparmor_audit_data *ad = aad_of_va(va); + char *buffer; + const char *path; if (ad->request & AA_USERNS_CREATE) audit_log_format(ab, " requested=\"userns_create\""); if (ad->denied & AA_USERNS_CREATE) audit_log_format(ab, " denied=\"userns_create\""); + + buffer = aa_get_buffer(false); + if (!buffer) + return; // OOM + path = get_current_exe_path(buffer, aa_g_path_max); + if (!IS_ERR(path)) + audit_log_format(ab, " execpath=\"%s\"", path); + aa_put_buffer(buffer); } int aa_profile_ns_perm(struct aa_profile *profile, From 48d5268e911abcf7674ec33c9b0b3e952be1175e Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 7 Jan 2026 11:48:54 -0800 Subject: [PATCH 12/34] apparmor: fix boolean argument in apparmor_mmap_file The previous value of GFP_ATOMIC is an int and not a bool, potentially resulting in UB when being assigned to a bool. In addition, the mmap hook is called outside of locks (i.e. in a non-atomic context), so we can pass a fixed constant value of false instead to common_mmap. Signed-off-by: Ryan Lee Signed-off-by: John Johansen --- security/apparmor/lsm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index a87cd60ed206..acca3d6efdbc 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -584,7 +584,7 @@ static int common_mmap(const char *op, struct file *file, unsigned long prot, static int apparmor_mmap_file(struct file *file, unsigned long reqprot, unsigned long prot, unsigned long flags) { - return common_mmap(OP_FMMAP, file, prot, flags, GFP_ATOMIC); + return common_mmap(OP_FMMAP, file, prot, flags, false); } static int apparmor_file_mprotect(struct vm_area_struct *vma, From c3f27ccdb2dce3f0f2814574d06017f46c11fa29 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 17 Jan 2026 23:40:03 -0800 Subject: [PATCH 13/34] apparmor: drop in_atomic flag in common_mmap, and common_file_perm with the previous changes to mmap the in_atomic flag is now always false, so drop it. Suggested-by: Tyler Hicks Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/lsm.c | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index acca3d6efdbc..e59e9bc7250b 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -520,8 +520,7 @@ static void apparmor_file_free_security(struct file *file) aa_put_label(rcu_access_pointer(ctx->label)); } -static int common_file_perm(const char *op, struct file *file, u32 mask, - bool in_atomic) +static int common_file_perm(const char *op, struct file *file, u32 mask) { struct aa_label *label; int error = 0; @@ -532,7 +531,7 @@ static int common_file_perm(const char *op, struct file *file, u32 mask, return -EACCES; label = __begin_current_label_crit_section(&needput); - error = aa_file_perm(op, current_cred(), label, file, mask, in_atomic); + error = aa_file_perm(op, current_cred(), label, file, mask, false); __end_current_label_crit_section(label, needput); return error; @@ -540,13 +539,12 @@ static int common_file_perm(const char *op, struct file *file, u32 mask, static int apparmor_file_receive(struct file *file) { - return common_file_perm(OP_FRECEIVE, file, aa_map_file_to_perms(file), - false); + return common_file_perm(OP_FRECEIVE, file, aa_map_file_to_perms(file)); } static int apparmor_file_permission(struct file *file, int mask) { - return common_file_perm(OP_FPERM, file, mask, false); + return common_file_perm(OP_FPERM, file, mask); } static int apparmor_file_lock(struct file *file, unsigned int cmd) @@ -556,11 +554,11 @@ static int apparmor_file_lock(struct file *file, unsigned int cmd) if (cmd == F_WRLCK) mask |= MAY_WRITE; - return common_file_perm(OP_FLOCK, file, mask, false); + return common_file_perm(OP_FLOCK, file, mask); } static int common_mmap(const char *op, struct file *file, unsigned long prot, - unsigned long flags, bool in_atomic) + unsigned long flags) { int mask = 0; @@ -578,21 +576,20 @@ static int common_mmap(const char *op, struct file *file, unsigned long prot, if (prot & PROT_EXEC) mask |= AA_EXEC_MMAP; - return common_file_perm(op, file, mask, in_atomic); + return common_file_perm(op, file, mask); } static int apparmor_mmap_file(struct file *file, unsigned long reqprot, unsigned long prot, unsigned long flags) { - return common_mmap(OP_FMMAP, file, prot, flags, false); + return common_mmap(OP_FMMAP, file, prot, flags); } static int apparmor_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot, unsigned long prot) { return common_mmap(OP_FMPROT, vma->vm_file, prot, - !(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0, - false); + !(vma->vm_flags & VM_SHARED) ? MAP_PRIVATE : 0); } #ifdef CONFIG_IO_URING From 9b829c0aa96e9385b1e9a308d3eb054b95fbeda2 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Wed, 7 Jan 2026 11:47:02 -0800 Subject: [PATCH 14/34] apparmor: account for in_atomic removal in common_file_perm If we are not in an atomic context in common_file_perm, then we don't have to use the atomic versions, resulting in improved performance outside of atomic contexts. Signed-off-by: Ryan Lee Signed-off-by: John Johansen --- security/apparmor/lsm.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index e59e9bc7250b..f47d60d8c40a 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -524,15 +524,14 @@ static int common_file_perm(const char *op, struct file *file, u32 mask) { struct aa_label *label; int error = 0; - bool needput; /* don't reaudit files closed during inheritance */ if (unlikely(file->f_path.dentry == aa_null.dentry)) return -EACCES; - label = __begin_current_label_crit_section(&needput); + label = begin_current_label_crit_section(); error = aa_file_perm(op, current_cred(), label, file, mask, false); - __end_current_label_crit_section(label, needput); + end_current_label_crit_section(label); return error; } From 74b7105e53e80a4072bd3e1a50be7aa15e3f0a01 Mon Sep 17 00:00:00 2001 From: Ryan Lee Date: Tue, 13 Jan 2026 09:35:57 -0800 Subject: [PATCH 15/34] apparmor: return -ENOMEM in unpack_perms_table upon alloc failure In policy_unpack.c:unpack_perms_table, the perms struct is allocated via kcalloc, with the position being reset if the allocation fails. However, the error path results in -EPROTO being retured instead of -ENOMEM. Fix this to return the correct error code. Reported-by: Zygmunt Krynicki Fixes: fd1b2b95a2117 ("apparmor: add the ability for policy to specify a permission table") Reviewed-by: Tyler Hicks Signed-off-by: Ryan Lee Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index b6e18ddff331..f86133f36f33 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -923,8 +923,10 @@ static ssize_t unpack_perms_table(struct aa_ext *e, struct aa_perms **perms) if (!aa_unpack_array(e, NULL, &size)) goto fail_reset; *perms = kcalloc(size, sizeof(struct aa_perms), GFP_KERNEL); - if (!*perms) - goto fail_reset; + if (!*perms) { + e->pos = pos; + return -ENOMEM; + } for (i = 0; i < size; i++) { if (!unpack_perm(e, version, &(*perms)[i])) goto fail; From e16eee7895502bc3c07043169940b005d44bba8f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 3 Aug 2025 22:07:52 -0700 Subject: [PATCH 16/34] apparmor: guard against free routines being called with a NULL aa_free_data() and free_attachment() don't guard against having a NULL parameter passed to them. Fix this. Reviewed-by: Ryan Lee Signed-off-by: John Johansen --- security/apparmor/policy.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index f2cef22ed729..a64cfd24cedc 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -232,6 +232,9 @@ static void aa_free_data(void *ptr, void *arg) { struct aa_data *data = ptr; + if (!ptr) + return; + kvfree_sensitive(data->data, data->size); kfree_sensitive(data->key); kfree_sensitive(data); @@ -241,6 +244,9 @@ static void free_attachment(struct aa_attachment *attach) { int i; + if (!attach) + return; + for (i = 0; i < attach->xattr_count; i++) kfree_sensitive(attach->xattrs[i]); kfree_sensitive(attach->xattrs); From 4a134723f9f1ad2f3621566259db673350d19cb1 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 13 Sep 2025 02:22:21 -0700 Subject: [PATCH 17/34] apparmor: move check for aa_null file to cover all cases files with a dentry pointing aa_null.dentry where already rejected as part of file_inheritance. Unfortunately the check in common_file_perm() is insufficient to cover all cases causing unnecessary audit messages without the original files context. Eg. [ 442.886474] audit: type=1400 audit(1704822661.616:329): apparmor="DENIED" operation="file_inherit" class="file" namespace="root//lxd-juju-98527a-0_" profile="snap.lxd.activate" name="/apparmor/.null" pid=9525 comm="snap-exec" Further examples of this are in the logs of https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2120439 https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/1952084 https://bugs.launchpad.net/snapd/+bug/2049099 These messages have no value and should not be sent to the logs. AppArmor was already filtering the out in some cases but the original patch did not catch all cases. Fix this by push the existing check down into two functions that should cover all cases. Link: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2122743 Fixes: 192ca6b55a86 ("apparmor: revalidate files during exec") Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/file.c | 12 ++++++++++-- security/apparmor/lsm.c | 4 ---- 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index b69fece45ade..c751f2774c59 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -155,8 +155,12 @@ static int path_name(const char *op, const struct cred *subj_cred, const char *info = NULL; int error; - error = aa_path_name(path, flags, buffer, name, &info, - labels_profile(label)->disconnected); + /* don't reaudit files closed during inheritance */ + if (unlikely(path->dentry == aa_null.dentry)) + error = -EACCES; + else + error = aa_path_name(path, flags, buffer, name, &info, + labels_profile(label)->disconnected); if (error) { fn_for_each_confined(label, profile, aa_audit_file(subj_cred, @@ -617,6 +621,10 @@ int aa_file_perm(const char *op, const struct cred *subj_cred, AA_BUG(!label); AA_BUG(!file); + /* don't reaudit files closed during inheritance */ + if (unlikely(file->f_path.dentry == aa_null.dentry)) + return -EACCES; + fctx = file_ctx(file); rcu_read_lock(); diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index f47d60d8c40a..8d5d9a966b71 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -525,10 +525,6 @@ static int common_file_perm(const char *op, struct file *file, u32 mask) struct aa_label *label; int error = 0; - /* don't reaudit files closed during inheritance */ - if (unlikely(file->f_path.dentry == aa_null.dentry)) - return -EACCES; - label = begin_current_label_crit_section(); error = aa_file_perm(op, current_cred(), label, file, mask, false); end_current_label_crit_section(label); From 1301b956190590ef7f64b321fd27c59907d9c271 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 2 Nov 2025 01:36:57 -0800 Subject: [PATCH 18/34] apparmor: fix label and profile debug macros The label and profile debug macros were not correctly pasting their var args. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/include/lib.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h index 7ca8a92c449c..73ec624fd8c4 100644 --- a/security/apparmor/include/lib.h +++ b/security/apparmor/include/lib.h @@ -47,9 +47,11 @@ extern struct aa_dfa *stacksplitdfa; #define AA_DEBUG_LABEL(LAB, X, fmt, args...) \ do { \ if ((LAB)->flags & FLAG_DEBUG1) \ - AA_DEBUG(X, fmt, args); \ + AA_DEBUG(X, fmt, ##args); \ } while (0) +#define AA_DEBUG_PROFILE(PROF, X, fmt...) AA_DEBUG_LABEL(&(PROF)->label, X, ##fmt) + #define AA_WARN(X) WARN((X), "APPARMOR WARN %s: %s\n", __func__, #X) #define AA_BUG(X, args...) \ From acf2a94ac4734962742398bed0cde156baf48244 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 8 Nov 2025 03:00:56 -0800 Subject: [PATCH 19/34] apparmor: refactor/cleanup cred helper fns. aa_cred_raw_label() and cred_label() now do the same things so consolidate to cred_label() Document the crit section use and constraints better and refactor __begin_current_label_crit_section() into a base fn __begin_cred_crit_section() and a wrapper that calls the base with current cred. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/include/cred.h | 104 +++++++++++++++++++++---------- 1 file changed, 71 insertions(+), 33 deletions(-) diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h index b028e4c13b6f..2b6098149b15 100644 --- a/security/apparmor/include/cred.h +++ b/security/apparmor/include/cred.h @@ -36,22 +36,6 @@ static inline void set_cred_label(const struct cred *cred, *blob = label; } -/** - * aa_cred_raw_label - obtain cred's label - * @cred: cred to obtain label from (NOT NULL) - * - * Returns: confining label - * - * does NOT increment reference count - */ -static inline struct aa_label *aa_cred_raw_label(const struct cred *cred) -{ - struct aa_label *label = cred_label(cred); - - AA_BUG(!label); - return label; -} - /** * aa_get_newest_cred_label - obtain the newest label on a cred * @cred: cred to obtain label from (NOT NULL) @@ -60,13 +44,13 @@ static inline struct aa_label *aa_cred_raw_label(const struct cred *cred) */ static inline struct aa_label *aa_get_newest_cred_label(const struct cred *cred) { - return aa_get_newest_label(aa_cred_raw_label(cred)); + return aa_get_newest_label(cred_label(cred)); } static inline struct aa_label *aa_get_newest_cred_label_condref(const struct cred *cred, bool *needput) { - struct aa_label *l = aa_cred_raw_label(cred); + struct aa_label *l = cred_label(cred); if (unlikely(label_is_stale(l))) { *needput = true; @@ -93,7 +77,7 @@ static inline void aa_put_label_condref(struct aa_label *l, bool needput) */ static inline struct aa_label *aa_current_raw_label(void) { - return aa_cred_raw_label(current_cred()); + return cred_label(current_cred()); } /** @@ -115,19 +99,81 @@ static inline struct aa_label *aa_get_current_label(void) } /** - * __end_current_label_crit_section - end crit section begun with __begin_... - * @label: label obtained from __begin_current_label_crit_section - * @needput: output: bool set by __begin_current_label_crit_section + * __end_cred_crit_section - end crit section begun with __begin_... + * @label: label obtained from __begin_cred_crit_section + * @needput: output: bool set by __begin_cred_crit_section * - * Returns: label to use for this crit section + * While the cred passed to __begin is guaranteed to not change + * and the cred and label could be passed here instead of needput + * using needput with a local var makes it easier for the compiler + * and processor to optimize and speculatively execute the comparison + * than chasing a pointer in the cred struct. */ -static inline void __end_current_label_crit_section(struct aa_label *label, +static inline void __end_cred_crit_section(struct aa_label *label, bool needput) { if (unlikely(needput)) aa_put_label(label); } +/** + * __begin_cred_crit_section - @cred's confining label + * @cred: current's cred to start a crit section on its label + * @needput: store whether the label needs to be put when ending crit section + * + * Returns: up to date confining label or the ns unconfined label (NOT NULL) + * + * safe to call inside locks + * + * The returned reference must be put with __end_cred_crit_section() + * This must NOT be used if the task cred could be updated within the + * critical section between + * __begin_cred_crit_section() .. __end_cred_crit_section() + * + * The crit section is an optimization to avoid having to get and put + * the newest version of the label. While the cred won't change and + * hence the label it contains won't change, the newest version of the + * label can. During the crit section the newest versions of the label + * will be used until the end of the crit section. + * + * If the label has not been updated at the start of the crit section + * no refcount is taken, the cred's refcount is enough to hold the + * label for the duration of the crit section. + * + * If the label has been updated then a refcount will be taken and the + * newest version of the label will be returned. While the cred label + * and the returned label could be compared at the end of the crit + * section, needput is used because it allows better optimization by + * the compiler and the processor's speculative execution. + */ +static inline struct aa_label *__begin_cred_crit_section(const struct cred *cred, + bool *needput) +{ + struct aa_label *label = cred_label(cred); + + if (label_is_stale(label)) { + *needput = true; + return aa_get_newest_label(label); + } + + *needput = false; + return label; +} + +/** + * __end_current_label_crit_section - end crit section begun with __begin_... + * @label: label obtained from __begin_current_label_crit_section + * @needput: output: bool set by __begin_current_label_crit_section + * + * wrapper around __end_cred_crit_section() to pair nicely with + * __begin_current_label_crit_section() + */ +static inline void __end_current_label_crit_section(struct aa_label *label, + bool needput) +{ + __end_cred_crit_section(label, needput); +} + /** * end_current_label_crit_section - put a reference found with begin_current_label.. * @label: label reference to put @@ -157,15 +203,7 @@ static inline void end_current_label_crit_section(struct aa_label *label) */ static inline struct aa_label *__begin_current_label_crit_section(bool *needput) { - struct aa_label *label = aa_current_raw_label(); - - if (label_is_stale(label)) { - *needput = true; - return aa_get_newest_label(label); - } - - *needput = false; - return label; + return __begin_cred_crit_section(current_cred(), needput); } /** From 6ca56813f4a589f536adceb42882855d91fb1125 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sun, 9 Nov 2025 14:16:54 -0800 Subject: [PATCH 20/34] apparmor: fix rlimit for posix cpu timers Posix cpu timers requires an additional step beyond setting the rlimit. Refactor the code so its clear when what code is setting the limit and conditionally update the posix cpu timers when appropriate. Fixes: baa73d9e478ff ("posix-timers: Make them configurable") Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/resource.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/security/apparmor/resource.c b/security/apparmor/resource.c index 8e80db3ae21c..64212b39ba4b 100644 --- a/security/apparmor/resource.c +++ b/security/apparmor/resource.c @@ -196,6 +196,11 @@ void __aa_transition_rlimits(struct aa_label *old_l, struct aa_label *new_l) rules->rlimits.limits[j].rlim_max); /* soft limit should not exceed hard limit */ rlim->rlim_cur = min(rlim->rlim_cur, rlim->rlim_max); + if (j == RLIMIT_CPU && + rlim->rlim_cur != RLIM_INFINITY && + IS_ENABLED(CONFIG_POSIX_TIMERS)) + (void) update_rlimit_cpu(current->group_leader, + rlim->rlim_cur); } } } From 9f79b1cee91b3591a9b8fc0b3534ec966b8e463f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 13 Sep 2025 20:49:11 -0700 Subject: [PATCH 21/34] apparmor: fix fast path cache check for unix sockets The fast path cache check is incorrect forcing more slow path revalidations than necessary, because the unix logic check is inverted. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/file.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index c751f2774c59..694e157149e8 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -572,8 +572,7 @@ static bool __file_is_delegated(struct aa_label *obj_label) return unconfined(obj_label); } -static bool __unix_needs_revalidation(struct file *file, struct aa_label *label, - u32 request) +static bool __is_unix_file(struct file *file) { struct socket *sock = (struct socket *) file->private_data; @@ -581,23 +580,31 @@ static bool __unix_needs_revalidation(struct file *file, struct aa_label *label, if (!S_ISSOCK(file_inode(file)->i_mode)) return false; - if (request & NET_PEER_MASK) - return false; /* sock and sock->sk can be NULL for sockets being set up or torn down */ if (!sock || !sock->sk) return false; - if (sock->sk->sk_family == PF_UNIX) { - struct aa_sk_ctx *ctx = aa_sock(sock->sk); - - if (rcu_access_pointer(ctx->peer) != - rcu_access_pointer(ctx->peer_lastupdate)) - return true; - return !__aa_subj_label_is_cached(rcu_dereference(ctx->label), - label); - } + if (sock->sk->sk_family == PF_UNIX) + return true; return false; } +static bool __unix_needs_revalidation(struct file *file, struct aa_label *label, + u32 request) +{ + struct socket *sock = (struct socket *) file->private_data; + + AA_BUG(!__is_unix_file(file)); + lockdep_assert_in_rcu_read_lock(); + + struct aa_sk_ctx *skctx = aa_sock(sock->sk); + + if (rcu_access_pointer(skctx->peer) != + rcu_access_pointer(skctx->peer_lastupdate)) + return true; + + return !__aa_subj_label_is_cached(rcu_dereference(skctx->label), label); +} + /** * aa_file_perm - do permission revalidation check & audit for @file * @op: operation being checked @@ -640,7 +647,7 @@ int aa_file_perm(const char *op, const struct cred *subj_cred, */ denied = request & ~fctx->allow; if (unconfined(label) || __file_is_delegated(flabel) || - __unix_needs_revalidation(file, label, request) || + (!denied && __is_unix_file(file) && !__unix_needs_revalidation(file, label, request)) || (!denied && __aa_subj_label_is_cached(label, flabel))) { rcu_read_unlock(); goto done; From b2e27be2948f2f8c38421cd554b5fc9383215648 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 14 Nov 2025 00:14:36 -0800 Subject: [PATCH 22/34] apparmor: remove apply_modes_to_perms from label_match The modes shouldn't be applied at the point of label match, it just results in them being applied multiple times. Instead they should be applied after which is already being done by all callers so it can just be dropped from label_match. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/label.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 913678f199c3..02ee128f53d1 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1317,7 +1317,6 @@ next: goto fail; } *perms = *aa_lookup_perms(rules->policy, state); - aa_apply_modes_to_perms(profile, perms); if ((perms->allow & request) != request) return -EACCES; @@ -1370,7 +1369,6 @@ static int label_components_match(struct aa_profile *profile, next: tmp = *aa_lookup_perms(rules->policy, state); - aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum(perms, &tmp); label_for_each_cont(i, label, tp) { if (!aa_ns_visible(profile->ns, tp->ns, subns)) @@ -1379,7 +1377,6 @@ next: if (!state) goto fail; tmp = *aa_lookup_perms(rules->policy, state); - aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum(perms, &tmp); } From a4c9efa4dbad6dacad6e8b274e30e814c8353097 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 13 Nov 2025 23:59:38 -0800 Subject: [PATCH 23/34] apparmor: make label_match return a consistent value compound match is inconsistent in returning a state or an integer error this is problemati if the error is ever used as a state in the state machine Fixes: f1bd904175e81 ("apparmor: add the base fns() for domain labels") Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/label.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 02ee128f53d1..1d3fa5c28d97 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1278,7 +1278,7 @@ static inline aa_state_t match_component(struct aa_profile *profile, * @request: permissions to request * @perms: perms struct to set * - * Returns: 0 on success else ERROR + * Returns: state match stopped at or DFA_NOMATCH if aborted early * * For the label A//&B//&C this does the perm match for A//&B//&C * @perms should be preinitialized with allperms OR a previous permission @@ -1305,7 +1305,7 @@ static int label_compound_match(struct aa_profile *profile, /* no component visible */ *perms = allperms; - return 0; + return state; next: label_for_each_cont(i, label, tp) { @@ -1317,14 +1317,11 @@ next: goto fail; } *perms = *aa_lookup_perms(rules->policy, state); - if ((perms->allow & request) != request) - return -EACCES; - - return 0; + return state; fail: *perms = nullperms; - return state; + return DFA_NOMATCH; } /** @@ -1406,11 +1403,12 @@ int aa_label_match(struct aa_profile *profile, struct aa_ruleset *rules, struct aa_label *label, aa_state_t state, bool subns, u32 request, struct aa_perms *perms) { - int error = label_compound_match(profile, rules, label, state, subns, - request, perms); - if (!error) - return error; + aa_state_t tmp = label_compound_match(profile, rules, label, state, subns, + request, perms); + if ((perms->allow & request) == request) + return 0; + /* failed compound_match try component matches */ *perms = allperms; return label_components_match(profile, rules, label, state, subns, request, perms); From 796c146fa6c8289afc9e18004c21bfe05c75a487 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Thu, 25 Dec 2025 01:21:23 -0800 Subject: [PATCH 24/34] apparmor: split xxx_in_ns into its two separate semantic use cases This patch doesn't change current functionality, it switches the two uses of the in_ns fns and macros into the two semantically different cases they are used for. xxx_in_scope for checking mediation interaction between profiles xxx_in_view to determine which profiles are visible.The scope will always be a subset of the view as profiles that can not see each other can not interact. The split can not be completely done for label_match because it has to distinct uses matching permission against label in scope, and checking if a transition to a profile is allowed. The transition to a profile can include profiles that are in view but not in scope, so retain this distinction as a parameter. While at the moment the two uses are very similar, in the future there will be additional differences. So make sure the semantics differences are present in the code. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/af_unix.c | 2 +- security/apparmor/apparmorfs.c | 2 +- security/apparmor/domain.c | 58 +++++++++++++++++---------------- security/apparmor/include/lib.h | 19 +++++++++-- security/apparmor/label.c | 26 +++++++-------- 5 files changed, 61 insertions(+), 46 deletions(-) diff --git a/security/apparmor/af_unix.c b/security/apparmor/af_unix.c index ac0f4be791ec..fdb4a9f212c3 100644 --- a/security/apparmor/af_unix.c +++ b/security/apparmor/af_unix.c @@ -416,7 +416,7 @@ static int profile_peer_perm(struct aa_profile *profile, u32 request, unix_sk(sk), peer_addr, peer_addrlen, &p, &ad->info); - return fn_for_each_in_ns(peer_label, peerp, + return fn_for_each_in_scope(peer_label, peerp, match_label(profile, rules, state, request, peerp, p, ad)); } diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 91c1fcb78ac8..9b4f833e36cd 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -801,7 +801,7 @@ static ssize_t query_label(char *buf, size_t buf_len, perms = allperms; if (view_only) { - label_for_each_in_ns(i, labels_ns(label), label, profile) { + label_for_each_in_scope(i, labels_ns(label), label, profile) { profile_query_cb(profile, &perms, match_str, match_len); } } else { diff --git a/security/apparmor/domain.c b/security/apparmor/domain.c index ee2df25f355d..f02bf770f638 100644 --- a/security/apparmor/domain.c +++ b/security/apparmor/domain.c @@ -115,7 +115,7 @@ static inline aa_state_t match_component(struct aa_profile *profile, * @label: label to check access permissions for * @stack: whether this is a stacking request * @state: state to start match in - * @subns: whether to do permission checks on components in a subns + * @inview: whether to match labels in view or only in scope * @request: permissions to request * @perms: perms struct to set * @@ -127,7 +127,7 @@ static inline aa_state_t match_component(struct aa_profile *profile, */ static int label_compound_match(struct aa_profile *profile, struct aa_label *label, bool stack, - aa_state_t state, bool subns, u32 request, + aa_state_t state, bool inview, u32 request, struct aa_perms *perms) { struct aa_ruleset *rules = profile->label.rules[0]; @@ -135,9 +135,9 @@ static int label_compound_match(struct aa_profile *profile, struct label_it i; struct path_cond cond = { }; - /* find first subcomponent that is visible */ + /* find first subcomponent that is in view and going to be interated with */ label_for_each(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, tp, stack, state); if (!state) @@ -151,7 +151,7 @@ static int label_compound_match(struct aa_profile *profile, next: label_for_each_cont(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = aa_dfa_match(rules->file->dfa, state, "//&"); state = match_component(profile, tp, false, state); @@ -177,7 +177,7 @@ fail: * @label: label to check access permissions for * @stack: whether this is a stacking request * @start: state to start match in - * @subns: whether to do permission checks on components in a subns + * @inview: whether to match labels in view or only in scope * @request: permissions to request * @perms: an initialized perms struct to add accumulation to * @@ -189,7 +189,7 @@ fail: */ static int label_components_match(struct aa_profile *profile, struct aa_label *label, bool stack, - aa_state_t start, bool subns, u32 request, + aa_state_t start, bool inview, u32 request, struct aa_perms *perms) { struct aa_ruleset *rules = profile->label.rules[0]; @@ -201,7 +201,7 @@ static int label_components_match(struct aa_profile *profile, /* find first subcomponent to test */ label_for_each(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, tp, stack, start); if (!state) @@ -218,7 +218,7 @@ next: aa_apply_modes_to_perms(profile, &tmp); aa_perms_accum(perms, &tmp); label_for_each_cont(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, tp, stack, start); if (!state) @@ -245,26 +245,26 @@ fail: * @label: label to match (NOT NULL) * @stack: whether this is a stacking request * @state: state to start in - * @subns: whether to match subns components + * @inview: whether to match labels in view or only in scope * @request: permission request * @perms: Returns computed perms (NOT NULL) * * Returns: the state the match finished in, may be the none matching state */ static int label_match(struct aa_profile *profile, struct aa_label *label, - bool stack, aa_state_t state, bool subns, u32 request, + bool stack, aa_state_t state, bool inview, u32 request, struct aa_perms *perms) { int error; *perms = nullperms; - error = label_compound_match(profile, label, stack, state, subns, + error = label_compound_match(profile, label, stack, state, inview, request, perms); if (!error) return error; *perms = allperms; - return label_components_match(profile, label, stack, state, subns, + return label_components_match(profile, label, stack, state, inview, request, perms); } @@ -880,14 +880,16 @@ static struct aa_label *handle_onexec(const struct cred *subj_cred, AA_BUG(!bprm); AA_BUG(!buffer); - /* TODO: determine how much we want to loosen this */ - error = fn_for_each_in_ns(label, profile, + /* TODO: determine how much we want to loosen this + * only check profiles in scope for permission to change at exec + */ + error = fn_for_each_in_scope(label, profile, profile_onexec(subj_cred, profile, onexec, stack, bprm, buffer, cond, unsafe)); if (error) return ERR_PTR(error); - new = fn_label_build_in_ns(label, profile, GFP_KERNEL, + new = fn_label_build_in_scope(label, profile, GFP_KERNEL, stack ? aa_label_merge(&profile->label, onexec, GFP_KERNEL) : aa_get_newest_label(onexec), @@ -897,7 +899,7 @@ static struct aa_label *handle_onexec(const struct cred *subj_cred, return new; /* TODO: get rid of GLOBAL_ROOT_UID */ - error = fn_for_each_in_ns(label, profile, + error = fn_for_each_in_scope(label, profile, aa_audit_file(subj_cred, profile, &nullperms, OP_CHANGE_ONEXEC, AA_MAY_ONEXEC, bprm->filename, NULL, @@ -1123,7 +1125,7 @@ static struct aa_label *change_hat(const struct cred *subj_cred, /*find first matching hat */ for (i = 0; i < count && !hat; i++) { name = hats[i]; - label_for_each_in_ns(it, labels_ns(label), label, profile) { + label_for_each_in_scope(it, labels_ns(label), label, profile) { if (sibling && PROFILE_IS_HAT(profile)) { root = aa_get_profile_rcu(&profile->parent); } else if (!sibling && !PROFILE_IS_HAT(profile)) { @@ -1159,7 +1161,7 @@ outer_continue: * change_hat. */ name = NULL; - label_for_each_in_ns(it, labels_ns(label), label, profile) { + label_for_each_in_scope(it, labels_ns(label), label, profile) { if (!list_empty(&profile->base.profiles)) { info = "hat not found"; error = -ENOENT; @@ -1170,7 +1172,7 @@ outer_continue: error = -ECHILD; fail: - label_for_each_in_ns(it, labels_ns(label), label, profile) { + label_for_each_in_scope(it, labels_ns(label), label, profile) { /* * no target as it has failed to be found or built * @@ -1188,7 +1190,7 @@ fail: return ERR_PTR(error); build: - new = fn_label_build_in_ns(label, profile, GFP_KERNEL, + new = fn_label_build_in_scope(label, profile, GFP_KERNEL, build_change_hat(subj_cred, profile, name, sibling), aa_get_label(&profile->label)); @@ -1251,7 +1253,7 @@ int aa_change_hat(const char *hats[], int count, u64 token, int flags) bool empty = true; rcu_read_lock(); - label_for_each_in_ns(i, labels_ns(label), label, profile) { + label_for_each_in_scope(i, labels_ns(label), label, profile) { empty &= list_empty(&profile->base.profiles); } rcu_read_unlock(); @@ -1338,7 +1340,7 @@ kill: perms.kill = AA_MAY_CHANGEHAT; fail: - fn_for_each_in_ns(label, profile, + fn_for_each_in_scope(label, profile, aa_audit_file(subj_cred, profile, &perms, OP_CHANGE_HAT, AA_MAY_CHANGEHAT, NULL, NULL, target, GLOBAL_ROOT_UID, info, error)); @@ -1446,7 +1448,7 @@ int aa_change_profile(const char *fqname, int flags) */ stack = true; perms.audit = request; - (void) fn_for_each_in_ns(label, profile, + (void) fn_for_each_in_scope(label, profile, aa_audit_file(subj_cred, profile, &perms, op, request, auditname, NULL, target, GLOBAL_ROOT_UID, stack_msg, 0)); @@ -1492,7 +1494,7 @@ int aa_change_profile(const char *fqname, int flags) * * if (!stack) { */ - error = fn_for_each_in_ns(label, profile, + error = fn_for_each_in_scope(label, profile, change_profile_perms_wrapper(op, auditname, subj_cred, profile, target, stack, @@ -1506,7 +1508,7 @@ int aa_change_profile(const char *fqname, int flags) check: /* check if tracing task is allowed to trace target domain */ error = may_change_ptraced_domain(subj_cred, target, &info); - if (error && !fn_for_each_in_ns(label, profile, + if (error && !fn_for_each_in_scope(label, profile, COMPLAIN_MODE(profile))) goto audit; @@ -1522,7 +1524,7 @@ check: /* stacking is always a subset, so only check the nonstack case */ if (!stack) { - new = fn_label_build_in_ns(label, profile, GFP_KERNEL, + new = fn_label_build_in_scope(label, profile, GFP_KERNEL, aa_get_label(target), aa_get_label(&profile->label)); /* @@ -1565,7 +1567,7 @@ check: } audit: - error = fn_for_each_in_ns(label, profile, + error = fn_for_each_in_scope(label, profile, aa_audit_file(subj_cred, profile, &perms, op, request, auditname, NULL, new ? new : target, diff --git a/security/apparmor/include/lib.h b/security/apparmor/include/lib.h index 73ec624fd8c4..1c5d1f60f6a7 100644 --- a/security/apparmor/include/lib.h +++ b/security/apparmor/include/lib.h @@ -80,6 +80,19 @@ int aa_print_debug_params(char *buffer); /* Flag indicating whether initialization completed */ extern int apparmor_initialized; +/* semantic split of scope and view */ +#define aa_in_scope(SUBJ, OBJ) \ + aa_ns_visible(SUBJ, OBJ, false) + +#define aa_in_view(SUBJ, OBJ) \ + aa_ns_visible(SUBJ, OBJ, true) + +#define label_for_each_in_scope(I, NS, L, P) \ + label_for_each_in_ns(I, NS, L, P) + +#define fn_for_each_in_scope(L, P, FN) \ + fn_for_each_in_ns(L, P, FN) + /* fn's in lib */ const char *skipn_spaces(const char *str, size_t n); const char *aa_splitn_fqname(const char *fqname, size_t n, const char **ns_name, @@ -316,7 +329,7 @@ __done: \ }) -#define __fn_build_in_ns(NS, P, NS_FN, OTHER_FN) \ +#define __fn_build_in_scope(NS, P, NS_FN, OTHER_FN) \ ({ \ struct aa_label *__new; \ if ((P)->ns != (NS)) \ @@ -326,10 +339,10 @@ __done: \ (__new); \ }) -#define fn_label_build_in_ns(L, P, GFP, NS_FN, OTHER_FN) \ +#define fn_label_build_in_scope(L, P, GFP, NS_FN, OTHER_FN) \ ({ \ fn_label_build((L), (P), (GFP), \ - __fn_build_in_ns(labels_ns(L), (P), (NS_FN), (OTHER_FN))); \ + __fn_build_in_scope(labels_ns(L), (P), (NS_FN), (OTHER_FN))); \ }) #endif /* __AA_LIB_H */ diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 1d3fa5c28d97..ef3448ecdf5f 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1274,7 +1274,7 @@ static inline aa_state_t match_component(struct aa_profile *profile, * @rules: ruleset to search * @label: label to check access permissions for * @state: state to start match in - * @subns: whether to do permission checks on components in a subns + * @inview: whether to match labels in view or only in scope * @request: permissions to request * @perms: perms struct to set * @@ -1287,7 +1287,7 @@ static inline aa_state_t match_component(struct aa_profile *profile, static int label_compound_match(struct aa_profile *profile, struct aa_ruleset *rules, struct aa_label *label, - aa_state_t state, bool subns, u32 request, + aa_state_t state, bool inview, u32 request, struct aa_perms *perms) { struct aa_profile *tp; @@ -1295,7 +1295,7 @@ static int label_compound_match(struct aa_profile *profile, /* find first subcomponent that is visible */ label_for_each(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, rules, tp, state); if (!state) @@ -1309,7 +1309,7 @@ static int label_compound_match(struct aa_profile *profile, next: label_for_each_cont(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = aa_dfa_match(rules->policy->dfa, state, "//&"); state = match_component(profile, rules, tp, state); @@ -1330,7 +1330,7 @@ fail: * @rules: ruleset to search * @label: label to check access permissions for * @start: state to start match in - * @subns: whether to do permission checks on components in a subns + * @subns: whether to match labels in view or only in scope * @request: permissions to request * @perms: an initialized perms struct to add accumulation to * @@ -1343,7 +1343,7 @@ fail: static int label_components_match(struct aa_profile *profile, struct aa_ruleset *rules, struct aa_label *label, aa_state_t start, - bool subns, u32 request, + bool inview, u32 request, struct aa_perms *perms) { struct aa_profile *tp; @@ -1353,7 +1353,7 @@ static int label_components_match(struct aa_profile *profile, /* find first subcomponent to test */ label_for_each(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, rules, tp, start); if (!state) @@ -1368,7 +1368,7 @@ next: tmp = *aa_lookup_perms(rules->policy, state); aa_perms_accum(perms, &tmp); label_for_each_cont(i, label, tp) { - if (!aa_ns_visible(profile->ns, tp->ns, subns)) + if (!aa_ns_visible(profile->ns, tp->ns, inview)) continue; state = match_component(profile, rules, tp, start); if (!state) @@ -1393,24 +1393,24 @@ fail: * @rules: ruleset to search * @label: label to match (NOT NULL) * @state: state to start in - * @subns: whether to match subns components + * @subns: whether to match labels in view or only in scope * @request: permission request * @perms: Returns computed perms (NOT NULL) * * Returns: the state the match finished in, may be the none matching state */ int aa_label_match(struct aa_profile *profile, struct aa_ruleset *rules, - struct aa_label *label, aa_state_t state, bool subns, + struct aa_label *label, aa_state_t state, bool inview, u32 request, struct aa_perms *perms) { - aa_state_t tmp = label_compound_match(profile, rules, label, state, subns, - request, perms); + aa_state_t tmp = label_compound_match(profile, rules, label, state, + inview, request, perms); if ((perms->allow & request) == request) return 0; /* failed compound_match try component matches */ *perms = allperms; - return label_components_match(profile, rules, label, state, subns, + return label_components_match(profile, rules, label, state, inview, request, perms); } From 640cf2f09575c9dc344b3f7be2498d31e3923ead Mon Sep 17 00:00:00 2001 From: Zhengmian Hu Date: Mon, 19 Jan 2026 19:03:07 -0500 Subject: [PATCH 25/34] apparmor: avoid per-cpu hold underflow in aa_get_buffer When aa_get_buffer() pulls from the per-cpu list it unconditionally decrements cache->hold. If hold reaches 0 while count is still non-zero, the unsigned decrement wraps to UINT_MAX. This keeps hold non-zero for a very long time, so aa_put_buffer() never returns buffers to the global list, which can starve other CPUs and force repeated kmalloc(aa_g_path_max) allocations. Guard the decrement so hold never underflows. Fixes: ea9bae12d028 ("apparmor: cache buffers on percpu list if there is lock contention") Signed-off-by: Zhengmian Hu Signed-off-by: John Johansen --- security/apparmor/lsm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 8d5d9a966b71..9175fd677ef3 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -2137,7 +2137,8 @@ char *aa_get_buffer(bool in_atomic) if (!list_empty(&cache->head)) { aa_buf = list_first_entry(&cache->head, union aa_buffer, list); list_del(&aa_buf->list); - cache->hold--; + if (cache->hold) + cache->hold--; cache->count--; put_cpu_ptr(&aa_local_buffers); return &aa_buf->buffer[0]; From 0b6a6b72b329c34a13a13908d5e8df9fb46eaa1f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 20 Jan 2026 18:18:51 -0800 Subject: [PATCH 26/34] apparmor: document the buffer hold, add an overflow guard The buffer hold is a measure of contention, but it is tracked per cpu where the lock is a globabl resource. On some systems (eg. real time) there is no guarantee that the code will be on the same cpu pre, and post spinlock acquisition, nor that the buffer will be put back to the same percpu cache when we are done with it. Because of this the hold value can move asynchronous to the buffers on the cache, meaning it is possible to underflow, and potentially in really pathelogical cases overflow. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/lsm.c | 28 ++++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 9175fd677ef3..34fc458887d7 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -2125,6 +2125,23 @@ static int param_set_mode(const char *val, const struct kernel_param *kp) return 0; } +/* arbitrary cap on how long to hold buffer because contention was + * encountered before trying to put it back into the global pool + */ +#define MAX_HOLD_COUNT 64 + +/* the hold count is a heuristic for lock contention, and can be + * incremented async to actual buffer alloc/free. Because buffers + * may be put back onto a percpu cache different than the ->hold was + * added to the counts can be out of sync. Guard against underflow + * and overflow + */ +static void cache_hold_inc(unsigned int *hold) +{ + if (*hold > MAX_HOLD_COUNT) + (*hold)++; +} + char *aa_get_buffer(bool in_atomic) { union aa_buffer *aa_buf; @@ -2143,11 +2160,18 @@ char *aa_get_buffer(bool in_atomic) put_cpu_ptr(&aa_local_buffers); return &aa_buf->buffer[0]; } + /* exit percpu as spinlocks may sleep on realtime kernels */ put_cpu_ptr(&aa_local_buffers); if (!spin_trylock(&aa_buffers_lock)) { + /* had contention on lock so increase hold count. Doesn't + * really matter if recorded before or after the spin lock + * as there is no way to guarantee the buffer will be put + * back on the same percpu cache. Instead rely on holds + * roughly averaging out over time. + */ cache = get_cpu_ptr(&aa_local_buffers); - cache->hold += 1; + cache_hold_inc(&cache->hold); put_cpu_ptr(&aa_local_buffers); spin_lock(&aa_buffers_lock); } else { @@ -2213,7 +2237,7 @@ void aa_put_buffer(char *buf) } /* contention on global list, fallback to percpu */ cache = get_cpu_ptr(&aa_local_buffers); - cache->hold += 1; + cache_hold_inc(&cache->hold); } /* cache in percpu list */ From 859b725579718ad1b66a81237df9d786f38679da Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 21 Jan 2026 01:09:11 -0800 Subject: [PATCH 27/34] apparmor: cleanup remove unused percpu critical sections in buffer management There are two unused percpu critical sections in the buffer management code. These are remanents from when a more complex hold algorithm was used. Remove them, as they serve no purpose. Reviewed-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/lsm.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 34fc458887d7..dcd066e04d2b 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -2174,9 +2174,6 @@ char *aa_get_buffer(bool in_atomic) cache_hold_inc(&cache->hold); put_cpu_ptr(&aa_local_buffers); spin_lock(&aa_buffers_lock); - } else { - cache = get_cpu_ptr(&aa_local_buffers); - put_cpu_ptr(&aa_local_buffers); } retry: if (buffer_count > reserve_count || @@ -2231,8 +2228,6 @@ void aa_put_buffer(char *buf) list_add(&aa_buf->list, &aa_global_buffers); buffer_count++; spin_unlock(&aa_buffers_lock); - cache = get_cpu_ptr(&aa_local_buffers); - put_cpu_ptr(&aa_local_buffers); return; } /* contention on global list, fallback to percpu */ From 2f53da43b20389b78ecfdb30f6349753acecb59e Mon Sep 17 00:00:00 2001 From: Ryota Sakamoto Date: Sun, 25 Jan 2026 19:05:48 +0900 Subject: [PATCH 28/34] apparmor: add .kunitconfig Add .kunitconfig file to the AppArmor directory to enable easy execution of KUnit tests. AppArmor tests (CONFIG_SECURITY_APPARMOR_KUNIT_TEST) depend on CONFIG_SECURITY_APPARMOR which also depends on CONFIG_SECURITY and CONFIG_NET. Without explicitly enabling these configs in the .kunitconfig, developers will need to specify config manually. With the .kunitconfig, developers can run the tests: $ ./tools/testing/kunit/kunit.py run --kunitconfig security/apparmor Signed-off-by: Ryota Sakamoto Signed-off-by: John Johansen --- security/apparmor/.kunitconfig | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 security/apparmor/.kunitconfig diff --git a/security/apparmor/.kunitconfig b/security/apparmor/.kunitconfig new file mode 100644 index 000000000000..aa842a0266e9 --- /dev/null +++ b/security/apparmor/.kunitconfig @@ -0,0 +1,5 @@ +CONFIG_KUNIT=y +CONFIG_NET=y +CONFIG_SECURITY=y +CONFIG_SECURITY_APPARMOR=y +CONFIG_SECURITY_APPARMOR_KUNIT_TEST=y From df9ac55abd18628bd8cff687ea043660532a3654 Mon Sep 17 00:00:00 2001 From: Georgia Garcia Date: Thu, 29 Jan 2026 15:58:45 -0300 Subject: [PATCH 29/34] apparmor: fix invalid deref of rawdata when export_binary is unset If the export_binary parameter is disabled on runtime, profiles that were loaded before that will still have their rawdata stored in apparmorfs, with a symbolic link to the rawdata on the policy directory. When one of those profiles are replaced, the rawdata is set to NULL, but when trying to resolve the symbolic links to rawdata for that profile, it will try to dereference profile->rawdata->name when profile->rawdata is now NULL causing an oops. Fix it by checking if rawdata is set. [ 168.653080] BUG: kernel NULL pointer dereference, address: 0000000000000088 [ 168.657420] #PF: supervisor read access in kernel mode [ 168.660619] #PF: error_code(0x0000) - not-present page [ 168.663613] PGD 0 P4D 0 [ 168.665450] Oops: Oops: 0000 [#1] SMP NOPTI [ 168.667836] CPU: 1 UID: 0 PID: 1729 Comm: ls Not tainted 6.19.0-rc7+ #3 PREEMPT(voluntary) [ 168.672308] Hardware name: QEMU Ubuntu 24.04 PC (i440FX + PIIX, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014 [ 168.679327] RIP: 0010:rawdata_get_link_base.isra.0+0x23/0x330 [ 168.682768] Code: 90 90 90 90 90 90 90 0f 1f 44 00 00 55 48 89 e5 41 57 41 56 41 55 41 54 53 48 83 ec 18 48 89 55 d0 48 85 ff 0f 84 e3 01 00 00 <48> 83 3c 25 88 00 00 00 00 0f 84 d4 01 00 00 49 89 f6 49 89 cc e8 [ 168.689818] RSP: 0018:ffffcdcb8200fb80 EFLAGS: 00010282 [ 168.690871] RAX: ffffffffaee74ec0 RBX: 0000000000000000 RCX: ffffffffb0120158 [ 168.692251] RDX: ffffcdcb8200fbe0 RSI: ffff88c187c9fa80 RDI: ffff88c186c98a80 [ 168.693593] RBP: ffffcdcb8200fbc0 R08: 0000000000000000 R09: 0000000000000000 [ 168.694941] R10: 0000000000000000 R11: 0000000000000000 R12: ffff88c186c98a80 [ 168.696289] R13: 00007fff005aaa20 R14: 0000000000000080 R15: ffff88c188f4fce0 [ 168.697637] FS: 0000790e81c58280(0000) GS:ffff88c20a957000(0000) knlGS:0000000000000000 [ 168.699227] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 168.700349] CR2: 0000000000000088 CR3: 000000012fd3e000 CR4: 0000000000350ef0 [ 168.701696] Call Trace: [ 168.702325] [ 168.702995] rawdata_get_link_data+0x1c/0x30 [ 168.704145] vfs_readlink+0xd4/0x160 [ 168.705152] do_readlinkat+0x114/0x180 [ 168.706214] __x64_sys_readlink+0x1e/0x30 [ 168.708653] x64_sys_call+0x1d77/0x26b0 [ 168.709525] do_syscall_64+0x81/0x500 [ 168.710348] ? do_statx+0x72/0xb0 [ 168.711109] ? putname+0x3e/0x80 [ 168.711845] ? __x64_sys_statx+0xb7/0x100 [ 168.712711] ? x64_sys_call+0x10fc/0x26b0 [ 168.713577] ? do_syscall_64+0xbf/0x500 [ 168.714412] ? do_user_addr_fault+0x1d2/0x8d0 [ 168.715404] ? irqentry_exit+0xb2/0x740 [ 168.716359] ? exc_page_fault+0x90/0x1b0 [ 168.717307] entry_SYSCALL_64_after_hwframe+0x76/0x7e Fixes: 1180b4c757aab ("apparmor: fix dangling symlinks to policy rawdata after replacement") Signed-off-by: Georgia Garcia Signed-off-by: John Johansen --- security/apparmor/apparmorfs.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c index 9b4f833e36cd..5a848c1be056 100644 --- a/security/apparmor/apparmorfs.c +++ b/security/apparmor/apparmorfs.c @@ -1648,6 +1648,15 @@ static const char *rawdata_get_link_base(struct dentry *dentry, label = aa_get_label_rcu(&proxy->label); profile = labels_profile(label); + + /* rawdata can be null when aa_g_export_binary is unset during + * runtime and a profile is replaced + */ + if (!profile->rawdata) { + aa_put_label(label); + return ERR_PTR(-ENOENT); + } + depth = profile_depth(profile); target = gen_symlink_name(depth, profile->rawdata->name, name); aa_put_label(label); From 3734b9463bd4fb5ac350842db55e2e0ccbf1b7a5 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 2 Feb 2026 03:37:18 -0800 Subject: [PATCH 30/34] apparmor: fix kernel-doc comments for inview subns was renamed inview to better reflect the function of the flag. Unfortunately the kernel-doc was not properly updated in 2 places. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202602020737.vGCZFds1-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202602021427.PvvDjgyL-lkp@intel.com/ Fixes: 796c146fa6c82 ("apparmor: split xxx_in_ns into its two separate semantic use cases") Signed-off-by: John Johansen --- security/apparmor/label.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/security/apparmor/label.c b/security/apparmor/label.c index ef3448ecdf5f..0852fa95081e 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1330,7 +1330,7 @@ fail: * @rules: ruleset to search * @label: label to check access permissions for * @start: state to start match in - * @subns: whether to match labels in view or only in scope + * @inview: whether to match labels in view or only in scope * @request: permissions to request * @perms: an initialized perms struct to add accumulation to * @@ -1393,7 +1393,7 @@ fail: * @rules: ruleset to search * @label: label to match (NOT NULL) * @state: state to start in - * @subns: whether to match labels in view or only in scope + * @inview: whether to match labels in view or only in scope * @request: permission request * @perms: Returns computed perms (NOT NULL) * From 102ada7ca37ed8358bd70c6f8e6475ebacf6f76d Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 2 Feb 2026 03:45:21 -0800 Subject: [PATCH 31/34] apparmor: fix fmt string type error in process_strs_entry pointer subtraction has a type of int when using clang on hexagon, microblaze (and possibly other archs). We know the subtraction is postive so cast the expression to unsigned long to match what is in the fmt string. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202602021429.CcmWkR9K-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202602021427.PvvDjgyL-lkp@intel.com/ Closes: https://lore.kernel.org/oe-kbuild-all/202602021510.JPzX5zKb-lkp@intel.com/ Fixes: c140dcd1246bf ("apparmor: make str table more generic and be able to have multiple entries") 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 f86133f36f33..175445c09896 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -474,7 +474,7 @@ static int process_strs_entry(char *str, int size, bool multi) if (!*str) { AA_DEBUG(DEBUG_UNPACK, "starting with null save=%lu size %d c=%d", - str - save, size, c); + (unsigned long)(str - save), size, c); return -4; } if (isspace(*str)) @@ -545,7 +545,8 @@ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, c = process_strs_entry(str, size2, multi); if (c <= 0) { AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", - c, i, e->pos - saved_pos); + c, i, + (unsigned_long) e->pos - saved_pos); goto fail; } if (!multi && c > 1) { From 9058798652c8bc0584ed1fb0766a1015046c06e8 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 2 Feb 2026 04:12:02 -0800 Subject: [PATCH 32/34] apparmor: fix aa_label to return state from compount and component match aa-label_match is not correctly returning the state in all cases. The only reason this didn't cause a error is that all callers currently ignore the return value. Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202602020631.wXgZosyU-lkp@intel.com/ Fixes: a4c9efa4dbad6 ("apparmor: make label_match return a consistent value") Signed-off-by: John Johansen --- security/apparmor/label.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/security/apparmor/label.c b/security/apparmor/label.c index 0852fa95081e..03a92a52acf9 100644 --- a/security/apparmor/label.c +++ b/security/apparmor/label.c @@ -1334,7 +1334,7 @@ fail: * @request: permissions to request * @perms: an initialized perms struct to add accumulation to * - * Returns: 0 on success else ERROR + * Returns: the state the match finished in, may be the none matching state * * For the label A//&B//&C this does the perm match for each of A and B and C * @perms should be preinitialized with allperms OR a previous permission @@ -1362,7 +1362,7 @@ static int label_components_match(struct aa_profile *profile, } /* no subcomponents visible - no change in perms */ - return 0; + return state; next: tmp = *aa_lookup_perms(rules->policy, state); @@ -1378,13 +1378,13 @@ next: } if ((perms->allow & request) != request) - return -EACCES; + return DFA_NOMATCH; - return 0; + return state; fail: *perms = nullperms; - return -EACCES; + return DFA_NOMATCH; } /** @@ -1406,7 +1406,7 @@ int aa_label_match(struct aa_profile *profile, struct aa_ruleset *rules, aa_state_t tmp = label_compound_match(profile, rules, label, state, inview, request, perms); if ((perms->allow & request) == request) - return 0; + return tmp; /* failed compound_match try component matches */ *perms = allperms; From 1b51bd761599b84e963d9cb510e7c7d8fbf9d1ee Mon Sep 17 00:00:00 2001 From: John Johansen Date: Tue, 3 Feb 2026 04:47:32 -0800 Subject: [PATCH 33/34] apparmor: fix cast in format string DEBUG statement if debugging is enabled the DEBUG statement will fail do to a bad fat fingered cast. Fixes: 102ada7ca37ed ("apparmor: fix fmt string type error in process_strs_entry") Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 175445c09896..e68adf39771f 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -546,7 +546,7 @@ static int unpack_strs_table(struct aa_ext *e, const char *name, bool multi, if (c <= 0) { AA_DEBUG(DEBUG_UNPACK, "process_strs %d i %d pos %ld", c, i, - (unsigned_long) e->pos - saved_pos); + (unsigned long)(e->pos - saved_pos)); goto fail; } if (!multi && c > 1) { From 08020dbe3125e936429e7966bf072e08fa964f36 Mon Sep 17 00:00:00 2001 From: Massimiliano Pellizzer Date: Tue, 10 Feb 2026 18:15:38 +0100 Subject: [PATCH 34/34] apparmor: fix signedness bug in unpack_tags() Smatch static checker warning: security/apparmor/policy_unpack.c:966 unpack_pdb() warn: unsigned 'unpack_tags(e, &pdb->tags, info)' is never less than zero. unpack_tags() is declared with return type size_t (unsigned) but returns negative errno values on failure. The caller in unpack_pdb() tests the return with `< 0`, which is always false for an unsigned type, making error handling dead code. Malformed tag data would be silently accepted instead of causing a load failure. Change return type of unpack_tags() from size_t to int to match the functions's actual semantic. Fixes: 3d28e2397af7 ("apparmor: add support loading per permission tagging") Reported-by: Dan Carpenter Signed-off-by: Massimiliano Pellizzer Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index e68adf39771f..dc908e1f5a88 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -835,7 +835,7 @@ fail_reset: } -static size_t unpack_tags(struct aa_ext *e, struct aa_tags_struct *tags, +static int unpack_tags(struct aa_ext *e, struct aa_tags_struct *tags, const char **info) { int error = -EPROTO;