From 76862af5d1add618f0cc99868bc729925f9551d2 Mon Sep 17 00:00:00 2001 From: Randy Dunlap Date: Mon, 2 Jan 2023 12:45:12 -0800 Subject: [PATCH 01/11] apparmor: fix kernel-doc complaints Correct kernel-doc notation to placate kernel-doc W=1 warnings: security/apparmor/policy.c:439: warning: duplicate section name 'Return' security/apparmor/secid.c:57: warning: Cannot understand * security/apparmor/file.c:174: warning: cannot understand function prototype: 'struct aa_perms default_perms = ' Signed-off-by: Randy Dunlap Cc: John Johansen Cc: John Johansen Cc: apparmor@lists.ubuntu.com Cc: Paul Moore Cc: James Morris Cc: "Serge E. Hallyn" Signed-off-by: John Johansen --- security/apparmor/file.c | 2 +- security/apparmor/policy.c | 7 ++----- security/apparmor/secid.c | 3 +-- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/security/apparmor/file.c b/security/apparmor/file.c index cb3d3060d1049b..56061c9ab0188b 100644 --- a/security/apparmor/file.c +++ b/security/apparmor/file.c @@ -161,6 +161,7 @@ static int path_name(const char *op, struct aa_label *label, return 0; } +struct aa_perms default_perms = {}; /** * aa_lookup_fperms - convert dfa compressed perms to internal perms * @dfa: dfa to lookup perms for (NOT NULL) @@ -171,7 +172,6 @@ static int path_name(const char *op, struct aa_label *label, * * Returns: a pointer to a file permission set */ -struct aa_perms default_perms = {}; struct aa_perms *aa_lookup_fperms(struct aa_policydb *file_rules, aa_state_t state, struct path_cond *cond) { diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index 51e8184e0fec15..a8fcc7291a7531 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -430,11 +430,9 @@ static struct aa_policy *__lookup_parent(struct aa_ns *ns, * @hname: hierarchical profile name to find parent of (NOT NULL) * @gfp: type of allocation. * - * Returns: NULL on error, parent profile on success - * * Requires: ns mutex lock held * - * Returns: unrefcounted parent policy or NULL if error creating + * Return: unrefcounted parent policy on success or %NULL if error creating * place holder profiles. */ static struct aa_policy *__create_missing_ancestors(struct aa_ns *ns, @@ -828,7 +826,7 @@ bool aa_current_policy_admin_capable(struct aa_ns *ns) /** * aa_may_manage_policy - can the current task manage policy * @label: label to check if it can manage policy - * @op: the policy manipulation operation being done + * @mask: contains the policy manipulation operation being done * * Returns: 0 if the task is allowed to manipulate policy else error */ @@ -883,7 +881,6 @@ static struct aa_profile *__list_lookup_parent(struct list_head *lh, * __replace_profile - replace @old with @new on a list * @old: profile to be replaced (NOT NULL) * @new: profile to replace @old with (NOT NULL) - * @share_proxy: transfer @old->proxy to @new * * Will duplicate and refcount elements that @new inherits from @old * and will inherit @old children. diff --git a/security/apparmor/secid.c b/security/apparmor/secid.c index 24a0e23f1b2ba3..83d3d1e6d9dcb9 100644 --- a/security/apparmor/secid.c +++ b/security/apparmor/secid.c @@ -53,8 +53,7 @@ void aa_secid_update(u32 secid, struct aa_label *label) xa_unlock_irqrestore(&aa_secids, flags); } -/** - * +/* * see label for inverse aa_label_to_secid */ struct aa_label *aa_secid_to_label(u32 secid) From b54aebd4411134b525a82d663a26b2f135ecb7e8 Mon Sep 17 00:00:00 2001 From: Rae Moar Date: Fri, 27 Jan 2023 20:12:19 +0000 Subject: [PATCH 02/11] apparmor: fix use of strcpy in policy_unpack_test Replace the use of strcpy() in build_aa_ext_struct() in policy_unpack_test.c with strscpy(). strscpy() is the safer method to use to ensure the buffer does not overflow. This was found by kernel test robot: https://lore.kernel.org/all/202301040348.NbfVsXO0-lkp@intel.com/. Reported-by: kernel test robot Signed-off-by: Rae Moar Signed-off-by: John Johansen --- security/apparmor/policy_unpack_test.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c index e1bfdab524b799..5c9bde25e56df9 100644 --- a/security/apparmor/policy_unpack_test.c +++ b/security/apparmor/policy_unpack_test.c @@ -69,31 +69,30 @@ static struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf, *buf = AA_NAME; *(buf + 1) = strlen(TEST_STRING_NAME) + 1; - strcpy(buf + 3, TEST_STRING_NAME); + strscpy(buf + 3, TEST_STRING_NAME, e->end - (void *)(buf + 3)); buf = e->start + TEST_STRING_BUF_OFFSET; *buf = AA_STRING; *(buf + 1) = strlen(TEST_STRING_DATA) + 1; - strcpy(buf + 3, TEST_STRING_DATA); - + strscpy(buf + 3, TEST_STRING_DATA, e->end - (void *)(buf + 3)); buf = e->start + TEST_NAMED_U32_BUF_OFFSET; *buf = AA_NAME; *(buf + 1) = strlen(TEST_U32_NAME) + 1; - strcpy(buf + 3, TEST_U32_NAME); + strscpy(buf + 3, TEST_U32_NAME, e->end - (void *)(buf + 3)); *(buf + 3 + strlen(TEST_U32_NAME) + 1) = AA_U32; *((u32 *)(buf + 3 + strlen(TEST_U32_NAME) + 2)) = TEST_U32_DATA; buf = e->start + TEST_NAMED_U64_BUF_OFFSET; *buf = AA_NAME; *(buf + 1) = strlen(TEST_U64_NAME) + 1; - strcpy(buf + 3, TEST_U64_NAME); + strscpy(buf + 3, TEST_U64_NAME, e->end - (void *)(buf + 3)); *(buf + 3 + strlen(TEST_U64_NAME) + 1) = AA_U64; *((u64 *)(buf + 3 + strlen(TEST_U64_NAME) + 2)) = TEST_U64_DATA; buf = e->start + TEST_NAMED_BLOB_BUF_OFFSET; *buf = AA_NAME; *(buf + 1) = strlen(TEST_BLOB_NAME) + 1; - strcpy(buf + 3, TEST_BLOB_NAME); + strscpy(buf + 3, TEST_BLOB_NAME, e->end - (void *)(buf + 3)); *(buf + 3 + strlen(TEST_BLOB_NAME) + 1) = AA_BLOB; *(buf + 3 + strlen(TEST_BLOB_NAME) + 2) = TEST_BLOB_DATA_SIZE; memcpy(buf + 3 + strlen(TEST_BLOB_NAME) + 6, @@ -102,7 +101,7 @@ static struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf, buf = e->start + TEST_NAMED_ARRAY_BUF_OFFSET; *buf = AA_NAME; *(buf + 1) = strlen(TEST_ARRAY_NAME) + 1; - strcpy(buf + 3, TEST_ARRAY_NAME); + strscpy(buf + 3, TEST_ARRAY_NAME, e->end - (void *)(buf + 3)); *(buf + 3 + strlen(TEST_ARRAY_NAME) + 1) = AA_ARRAY; *((u16 *)(buf + 3 + strlen(TEST_ARRAY_NAME) + 2)) = TEST_ARRAY_SIZE; From 755a22c74345a7b4c18dbb86f553eeb7895a97c9 Mon Sep 17 00:00:00 2001 From: Yang Li Date: Fri, 3 Mar 2023 09:28:33 +0800 Subject: [PATCH 03/11] AppArmor: Fix some kernel-doc comments Make the description of @table to @strs in function unpack_trans_table() to silence the warnings: security/apparmor/policy_unpack.c:456: warning: Function parameter or member 'strs' not described in 'unpack_trans_table' security/apparmor/policy_unpack.c:456: warning: Excess function parameter 'table' description in 'unpack_trans_table' Reported-by: Abaci Robot Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4332 Signed-off-by: Yang Li 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 66915653108c63..70caa444d9f890 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -437,7 +437,7 @@ static struct aa_dfa *unpack_dfa(struct aa_ext *e, int flags) /** * unpack_trans_table - unpack a profile transition table * @e: serialized data extent information (NOT NULL) - * @table: str table to unpack to (NOT NULL) + * @strs: str table to unpack to (NOT NULL) * * Returns: true if table successfully unpacked or not present */ From 6d7467957ecdc9018fb860bb60738e997abeaecb Mon Sep 17 00:00:00 2001 From: Markus Elfring Date: Wed, 29 Mar 2023 11:50:44 +0200 Subject: [PATCH 04/11] apparmor: Return directly after a failed kzalloc() in two functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. Return directly after a call of the function “kzalloc” failed at the beginning in these function implementations. 2. Omit extra initialisations (for a few local variables) which became unnecessary with this refactoring. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring Signed-off-by: John Johansen --- security/apparmor/crypto.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/security/apparmor/crypto.c b/security/apparmor/crypto.c index b498ed3024610f..6724e2ff6da890 100644 --- a/security/apparmor/crypto.c +++ b/security/apparmor/crypto.c @@ -28,15 +28,15 @@ unsigned int aa_hash_size(void) char *aa_calc_hash(void *data, size_t len) { SHASH_DESC_ON_STACK(desc, apparmor_tfm); - char *hash = NULL; - int error = -ENOMEM; + char *hash; + int error; if (!apparmor_tfm) return NULL; hash = kzalloc(apparmor_hash_size, GFP_KERNEL); if (!hash) - goto fail; + return ERR_PTR(-ENOMEM); desc->tfm = apparmor_tfm; @@ -62,7 +62,7 @@ int aa_calc_profile_hash(struct aa_profile *profile, u32 version, void *start, size_t len) { SHASH_DESC_ON_STACK(desc, apparmor_tfm); - int error = -ENOMEM; + int error; __le32 le32_version = cpu_to_le32(version); if (!aa_g_hash_policy) @@ -73,7 +73,7 @@ int aa_calc_profile_hash(struct aa_profile *profile, u32 version, void *start, profile->hash = kzalloc(apparmor_hash_size, GFP_KERNEL); if (!profile->hash) - goto fail; + return -ENOMEM; desc->tfm = apparmor_tfm; From 000518bc5aef25d3f703592a0296d578c98b1517 Mon Sep 17 00:00:00 2001 From: Danila Chernetsov Date: Tue, 4 Apr 2023 19:05:49 +0000 Subject: [PATCH 05/11] apparmor: fix missing error check for rhashtable_insert_fast rhashtable_insert_fast() could return err value when memory allocation is failed. but unpack_profile() do not check values and this always returns success value. This patch just adds error check code. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: e025be0f26d5 ("apparmor: support querying extended trusted helper extra data") Signed-off-by: Danila Chernetsov Signed-off-by: John Johansen --- security/apparmor/policy_unpack.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 70caa444d9f890..22137fef91472f 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1035,8 +1035,13 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) goto fail; } - rhashtable_insert_fast(profile->data, &data->head, - profile->data->p); + if (rhashtable_insert_fast(profile->data, &data->head, + profile->data->p)) { + kfree_sensitive(data->key); + kfree_sensitive(data); + info = "failed to insert data to table"; + goto fail; + } } if (!aa_unpack_nameX(e, AA_STRUCTEND, NULL)) { From 6600e9f692e36e265ef0828f08337fa294bb330f Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 14 Apr 2023 00:24:47 -0700 Subject: [PATCH 06/11] apparmor: add missing failure check in compute_xmatch_perms Add check for failure to allocate the permission table. Fixes: caa9f579ca72 ("apparmor: isolate policy backwards compatibility to its own file") Signed-off-by: John Johansen --- security/apparmor/policy_compat.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/security/apparmor/policy_compat.c b/security/apparmor/policy_compat.c index 9e52e218bf3081..b2ec7fb04e831b 100644 --- a/security/apparmor/policy_compat.c +++ b/security/apparmor/policy_compat.c @@ -180,6 +180,8 @@ static struct aa_perms *compute_xmatch_perms(struct aa_dfa *xmatch) state_count = xmatch->tables[YYTD_ID_BASE]->td_lolen; /* DFAs are restricted from having a state_count of less than 2 */ perms = kvcalloc(state_count, sizeof(struct aa_perms), GFP_KERNEL); + if (!perms) + return NULL; /* zero init so skip the trap state (state == 0) */ for (state = 1; state < state_count; state++) From ba808cb5edfdf032db9e849e194d28169b6efbcd Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 11 May 2023 14:34:45 -0700 Subject: [PATCH 07/11] apparmor: aa_buffer: Convert 1-element array to flexible array In the ongoing effort to convert all fake flexible arrays to proper flexible arrays, replace aa_buffer's 1-element "buffer" member with a flexible array. Signed-off-by: Kees Cook Signed-off-by: John Johansen --- security/apparmor/lsm.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index c6728a62943737..8d2d31d0d5065e 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -46,7 +46,7 @@ int apparmor_initialized; union aa_buffer { struct list_head list; - char buffer[1]; + DECLARE_FLEX_ARRAY(char, buffer); }; #define RESERVE_COUNT 2 @@ -1649,7 +1649,7 @@ char *aa_get_buffer(bool in_atomic) list_del(&aa_buf->list); buffer_count--; spin_unlock(&aa_buffers_lock); - return &aa_buf->buffer[0]; + return aa_buf->buffer; } if (in_atomic) { /* @@ -1672,7 +1672,7 @@ char *aa_get_buffer(bool in_atomic) pr_warn_once("AppArmor: Failed to allocate a memory buffer.\n"); return NULL; } - return &aa_buf->buffer[0]; + return aa_buf->buffer; } void aa_put_buffer(char *buf) @@ -1749,7 +1749,7 @@ static int __init alloc_buffers(void) destroy_buffers(); return -ENOMEM; } - aa_put_buffer(&aa_buf->buffer[0]); + aa_put_buffer(aa_buf->buffer); } return 0; } From 0bac2002b397fda7c9ea81ee0b06a02242958107 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Fri, 10 Mar 2023 15:59:45 -0800 Subject: [PATCH 08/11] apparmor: fix policy_compat permission remap with extended permissions If the extended permission table is present we should not be attempting to do a compat_permission remap as the compat_permissions are not stored in the dfa accept states. Fixes: fd1b2b95a211 ("apparmor: add the ability for policy to specify a permission table") Signed-off-by: John Johansen Reviewed-by: Jon Tourville --- security/apparmor/policy_unpack.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 22137fef91472f..a357c7b05276cb 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -849,10 +849,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) } profile->attach.xmatch_len = tmp; profile->attach.xmatch.start[AA_CLASS_XMATCH] = DFA_START; - error = aa_compat_map_xmatch(&profile->attach.xmatch); - if (error) { - info = "failed to convert xmatch permission table"; - goto fail; + if (!profile->attach.xmatch.perms) { + error = aa_compat_map_xmatch(&profile->attach.xmatch); + if (error) { + info = "failed to convert xmatch permission table"; + goto fail; + } } } @@ -972,10 +974,13 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) AA_CLASS_FILE); if (!aa_unpack_nameX(e, AA_STRUCTEND, NULL)) goto fail; - error = aa_compat_map_policy(&rules->policy, e->version); - if (error) { - info = "failed to remap policydb permission table"; - goto fail; + if (!rules->policy.perms) { + error = aa_compat_map_policy(&rules->policy, + e->version); + if (error) { + info = "failed to remap policydb permission table"; + goto fail; + } } } else rules->policy.dfa = aa_get_dfa(nulldfa); @@ -985,10 +990,12 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) if (error) { goto fail; } else if (rules->file.dfa) { - error = aa_compat_map_file(&rules->file); - if (error) { - info = "failed to remap file permission table"; - goto fail; + if (!rules->file.perms) { + error = aa_compat_map_file(&rules->file); + if (error) { + info = "failed to remap file permission table"; + goto fail; + } } } else if (rules->policy.dfa && rules->policy.start[AA_CLASS_FILE]) { From 6f442d42c0d89876994a4a135eadf82b0e6ff6e4 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Mon, 17 Apr 2023 02:57:55 -0700 Subject: [PATCH 09/11] apparmor: fix profile verification and enable it The transition table size was not being set by compat mappings resulting in the profile verification code not being run. Unfortunately the checks were also buggy not being correctly updated from the old accept perms, to the new layout. Also indicate to userspace that the kernel has the permstable verification fixes. BugLink: http://bugs.launchpad.net/bugs/2017903 Fixes: 670f31774ab6 ("apparmor: verify permission table indexes") Signed-off-by: John Johansen Reviewed-by: Jon Tourville --- security/apparmor/policy_compat.c | 18 ++++++++++------ security/apparmor/policy_unpack.c | 34 ++++++++++++++----------------- 2 files changed, 27 insertions(+), 25 deletions(-) diff --git a/security/apparmor/policy_compat.c b/security/apparmor/policy_compat.c index b2ec7fb04e831b..7ff9184ace2942 100644 --- a/security/apparmor/policy_compat.c +++ b/security/apparmor/policy_compat.c @@ -146,7 +146,8 @@ static struct aa_perms compute_fperms_other(struct aa_dfa *dfa, * * Returns: remapped perm table */ -static struct aa_perms *compute_fperms(struct aa_dfa *dfa) +static struct aa_perms *compute_fperms(struct aa_dfa *dfa, + u32 *size) { aa_state_t state; unsigned int state_count; @@ -159,6 +160,7 @@ static struct aa_perms *compute_fperms(struct aa_dfa *dfa) table = kvcalloc(state_count * 2, sizeof(struct aa_perms), GFP_KERNEL); if (!table) return NULL; + *size = state_count * 2; /* zero init so skip the trap state (state == 0) */ for (state = 1; state < state_count; state++) { @@ -169,7 +171,8 @@ static struct aa_perms *compute_fperms(struct aa_dfa *dfa) return table; } -static struct aa_perms *compute_xmatch_perms(struct aa_dfa *xmatch) +static struct aa_perms *compute_xmatch_perms(struct aa_dfa *xmatch, + u32 *size) { struct aa_perms *perms; int state; @@ -182,6 +185,7 @@ static struct aa_perms *compute_xmatch_perms(struct aa_dfa *xmatch) perms = kvcalloc(state_count, sizeof(struct aa_perms), GFP_KERNEL); if (!perms) return NULL; + *size = state_count; /* zero init so skip the trap state (state == 0) */ for (state = 1; state < state_count; state++) @@ -242,7 +246,8 @@ static struct aa_perms compute_perms_entry(struct aa_dfa *dfa, return perms; } -static struct aa_perms *compute_perms(struct aa_dfa *dfa, u32 version) +static struct aa_perms *compute_perms(struct aa_dfa *dfa, u32 version, + u32 *size) { unsigned int state; unsigned int state_count; @@ -255,6 +260,7 @@ static struct aa_perms *compute_perms(struct aa_dfa *dfa, u32 version) table = kvcalloc(state_count, sizeof(struct aa_perms), GFP_KERNEL); if (!table) return NULL; + *size = state_count; /* zero init so skip the trap state (state == 0) */ for (state = 1; state < state_count; state++) @@ -289,7 +295,7 @@ static void remap_dfa_accept(struct aa_dfa *dfa, unsigned int factor) /* TODO: merge different dfa mappings into single map_policy fn */ int aa_compat_map_xmatch(struct aa_policydb *policy) { - policy->perms = compute_xmatch_perms(policy->dfa); + policy->perms = compute_xmatch_perms(policy->dfa, &policy->size); if (!policy->perms) return -ENOMEM; @@ -300,7 +306,7 @@ int aa_compat_map_xmatch(struct aa_policydb *policy) int aa_compat_map_policy(struct aa_policydb *policy, u32 version) { - policy->perms = compute_perms(policy->dfa, version); + policy->perms = compute_perms(policy->dfa, version, &policy->size); if (!policy->perms) return -ENOMEM; @@ -311,7 +317,7 @@ int aa_compat_map_policy(struct aa_policydb *policy, u32 version) int aa_compat_map_file(struct aa_policydb *policy) { - policy->perms = compute_fperms(policy->dfa); + policy->perms = compute_fperms(policy->dfa, &policy->size); if (!policy->perms) return -ENOMEM; diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index a357c7b05276cb..2a50d3237ee6c6 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1135,22 +1135,16 @@ static int verify_header(struct aa_ext *e, int required, const char **ns) return 0; } -static bool verify_xindex(int xindex, int table_size) -{ - int index, xtype; - xtype = xindex & AA_X_TYPE_MASK; - index = xindex & AA_X_INDEX_MASK; - if (xtype == AA_X_TABLE && index >= table_size) - return false; - return true; -} - -/* verify dfa xindexes are in range of transition tables */ -static bool verify_dfa_xindex(struct aa_dfa *dfa, int table_size) +/** + * verify_dfa_accept_xindex - verify accept indexes are in range of perms table + * @dfa: the dfa to check accept indexes are in range + * table_size: the permission table size the indexes should be within + */ +static bool verify_dfa_accept_index(struct aa_dfa *dfa, int table_size) { int i; for (i = 0; i < dfa->tables[YYTD_ID_ACCEPT]->td_lolen; i++) { - if (!verify_xindex(ACCEPT_TABLE(dfa)[i], table_size)) + if (ACCEPT_TABLE(dfa)[i] >= table_size) return false; } return true; @@ -1187,11 +1181,13 @@ static bool verify_perms(struct aa_policydb *pdb) if (!verify_perm(&pdb->perms[i])) return false; /* verify indexes into str table */ - if (pdb->perms[i].xindex >= pdb->trans.size) + if ((pdb->perms[i].xindex & AA_X_TYPE_MASK) == AA_X_TABLE && + (pdb->perms[i].xindex & AA_X_INDEX_MASK) >= pdb->trans.size) return false; - if (pdb->perms[i].tag >= pdb->trans.size) + if (pdb->perms[i].tag && pdb->perms[i].tag >= pdb->trans.size) return false; - if (pdb->perms[i].label >= pdb->trans.size) + if (pdb->perms[i].label && + pdb->perms[i].label >= pdb->trans.size) return false; } @@ -1213,10 +1209,10 @@ static int verify_profile(struct aa_profile *profile) if (!rules) return 0; - if ((rules->file.dfa && !verify_dfa_xindex(rules->file.dfa, - rules->file.trans.size)) || + if ((rules->file.dfa && !verify_dfa_accept_index(rules->file.dfa, + rules->file.size)) || (rules->policy.dfa && - !verify_dfa_xindex(rules->policy.dfa, rules->policy.trans.size))) { + !verify_dfa_accept_index(rules->policy.dfa, rules->policy.size))) { audit_iface(profile, NULL, NULL, "Unpack: Invalid named transition", NULL, -EPROTO); return -EPROTO; From ec6851ae0ab4587e610e260ddda75f92f3389f91 Mon Sep 17 00:00:00 2001 From: John Johansen Date: Sat, 15 Apr 2023 00:50:32 -0700 Subject: [PATCH 10/11] apparmor: fix: kzalloc perms tables for shared dfas Currently the permstables of the shared dfas are not shared, and need to be allocated and copied. In the future this should be addressed with a larger rework on dfa and pdb ref counts and structure sharing. BugLink: http://bugs.launchpad.net/bugs/2017903 Fixes: 217af7e2f4de ("apparmor: refactor profile rules and attachments") Cc: stable@vger.kernel.org Signed-off-by: John Johansen Reviewed-by: Jon Tourville --- security/apparmor/policy.c | 13 +++++++++++++ security/apparmor/policy_unpack.c | 26 ++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/security/apparmor/policy.c b/security/apparmor/policy.c index a8fcc7291a7531..b38f7b2a5e1d5d 100644 --- a/security/apparmor/policy.c +++ b/security/apparmor/policy.c @@ -589,7 +589,15 @@ struct aa_profile *aa_alloc_null(struct aa_profile *parent, const char *name, profile->label.flags |= FLAG_NULL; rules = list_first_entry(&profile->rules, typeof(*rules), list); rules->file.dfa = aa_get_dfa(nulldfa); + rules->file.perms = kcalloc(2, sizeof(struct aa_perms), GFP_KERNEL); + if (!rules->file.perms) + goto fail; + rules->file.size = 2; rules->policy.dfa = aa_get_dfa(nulldfa); + rules->policy.perms = kcalloc(2, sizeof(struct aa_perms), GFP_KERNEL); + if (!rules->policy.perms) + goto fail; + rules->policy.size = 2; if (parent) { profile->path_flags = parent->path_flags; @@ -600,6 +608,11 @@ struct aa_profile *aa_alloc_null(struct aa_profile *parent, const char *name, } return profile; + +fail: + aa_free_profile(profile); + + return NULL; } /** diff --git a/security/apparmor/policy_unpack.c b/security/apparmor/policy_unpack.c index 2a50d3237ee6c6..f171f8a8ebd57c 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -982,9 +982,14 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) goto fail; } } - } else + } else { rules->policy.dfa = aa_get_dfa(nulldfa); - + rules->policy.perms = kcalloc(2, sizeof(struct aa_perms), + GFP_KERNEL); + if (!rules->policy.perms) + goto fail; + rules->policy.size = 2; + } /* get file rules */ error = unpack_pdb(e, &rules->file, false, true, &info); if (error) { @@ -1001,9 +1006,22 @@ static struct aa_profile *unpack_profile(struct aa_ext *e, char **ns_name) rules->policy.start[AA_CLASS_FILE]) { rules->file.dfa = aa_get_dfa(rules->policy.dfa); rules->file.start[AA_CLASS_FILE] = rules->policy.start[AA_CLASS_FILE]; - } else + rules->file.perms = kcalloc(rules->policy.size, + sizeof(struct aa_perms), + GFP_KERNEL); + if (!rules->file.perms) + goto fail; + memcpy(rules->file.perms, rules->policy.perms, + rules->policy.size * sizeof(struct aa_perms)); + rules->file.size = rules->policy.size; + } else { rules->file.dfa = aa_get_dfa(nulldfa); - + rules->file.perms = kcalloc(2, sizeof(struct aa_perms), + GFP_KERNEL); + if (!rules->file.perms) + goto fail; + rules->file.size = 2; + } error = -EPROTO; if (aa_unpack_nameX(e, AA_STRUCT, "data")) { info = "out of memory"; From 3f069c4c643225f2b96b4b3f8c30e4445f079d2e Mon Sep 17 00:00:00 2001 From: John Johansen Date: Wed, 21 Jun 2023 02:04:58 -0700 Subject: [PATCH 11/11] apparmor: Fix kernel-doc header for verify_dfa_accept_index Reported-by: kernel test robot Closes: https://lore.kernel.org/oe-kbuild-all/202306141934.UKmM9bFX-lkp@intel.com/ 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 f171f8a8ebd57c..35ec2d9b6064e9 100644 --- a/security/apparmor/policy_unpack.c +++ b/security/apparmor/policy_unpack.c @@ -1154,7 +1154,7 @@ static int verify_header(struct aa_ext *e, int required, const char **ns) } /** - * verify_dfa_accept_xindex - verify accept indexes are in range of perms table + * verify_dfa_accept_index - verify accept indexes are in range of perms table * @dfa: the dfa to check accept indexes are in range * table_size: the permission table size the indexes should be within */