Re: [PATCH v10 26/27] ima: Limit number of policy rules in non-init_ima_ns
From: Mimi Zohar <zohar@linux.ibm.com>
Date: 2022-02-23 15:39:22
Also in:
linux-integrity, lkml
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
Limit the number of policy rules one can set in non-init_ima_ns to a hardcoded 1024 rules. If the user attempts to exceed this limit by setting too many additional rules, emit an audit message with the cause 'too-many-rules' and simply ignore the newly added rules.
This paragraph describes 'what' you're doing, not 'why'. Please prefix this paragraph with a short, probably one sentence, reason for the change.
Switch the accounting for the memory allocated for IMA policy rules to GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect.
Does this change affect the existing IMA policy rules for init_ima_ns?
quoted hunk ↗ jump to hunk
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com> --- security/integrity/ima/ima_fs.c | 20 ++++++++++++++------ security/integrity/ima/ima_policy.c | 23 ++++++++++++++++++----- 2 files changed, 32 insertions(+), 11 deletions(-)diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 58d80884880f..cd102bbd4677 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c@@ -410,24 +410,32 @@ static int ima_release_policy(struct inode *inode, struct file *file) { struct ima_namespace *ns = &init_ima_ns; const char *cause = ns->valid_policy ? "completed" : "failed"; + int err = 0; if ((file->f_flags & O_ACCMODE) == O_RDONLY) return seq_release(inode, file); - if (ns->valid_policy && ima_check_policy(ns) < 0) { - cause = "failed"; - ns->valid_policy = 0; + if (ns->valid_policy) { + err = ima_check_policy(ns); + if (err < 0) { + if (err == -ENOSPC)
Perhaps "EDQUOT" would be more appropriate.
+ cause = "too-many-rules";
+ else
+ cause = "failed";
+ ns->valid_policy = 0;
+ }
}
pr_info("policy update %s\n", cause);
- integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
- "policy_update", cause, !ns->valid_policy, 0);
+ integrity_audit_message(AUDIT_INTEGRITY_STATUS, NULL, NULL,
+ "policy_update", cause, !ns->valid_policy, 0,
+ -err);The 'err' value is already properly set. No need for the minus sign.
quoted hunk ↗ jump to hunk
if (!ns->valid_policy) { ima_delete_rules(ns); ns->valid_policy = 1; clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags); - return 0; + return err; } ima_update_policy(ns);diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index acb4c36e539f..3f84d04f101d 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c@@ -305,7 +305,8 @@ static struct ima_rule_opt_list *ima_alloc_rule_opt_list(const substring_t *src) return ERR_PTR(-EINVAL); } - opt_list = kzalloc(struct_size(opt_list, items, count), GFP_KERNEL); + opt_list = kzalloc(struct_size(opt_list, items, count), + GFP_KERNEL_ACCOUNT); if (!opt_list) { kfree(src_copy); return ERR_PTR(-ENOMEM);@@ -379,7 +380,7 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_namespace *ns, * Immutable elements are copied over as pointers and data; only * lsm rules can change */ - nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL); + nentry = kmemdup(entry, sizeof(*nentry), GFP_KERNEL_ACCOUNT); if (!nentry) return NULL;@@ -826,7 +827,7 @@ static void add_rules(struct ima_namespace *ns, if (policy_rule & IMA_CUSTOM_POLICY) { entry = kmemdup(&entries[i], sizeof(*entry), - GFP_KERNEL); + GFP_KERNEL_ACCOUNT); if (!entry) continue;@@ -863,7 +864,7 @@ static int __init ima_init_arch_policy(struct ima_namespace *ns) ns->arch_policy_entry = kcalloc(arch_entries + 1, sizeof(*ns->arch_policy_entry), - GFP_KERNEL); + GFP_KERNEL_ACCOUNT); if (!ns->arch_policy_entry) return 0;@@ -975,8 +976,20 @@ void __init ima_init_policy(struct ima_namespace *ns) /* Make sure we have a valid policy, at least containing some rules. */ int ima_check_policy(struct ima_namespace *ns) { + struct ima_rule_entry *entry; + size_t len1 = 0; + size_t len2 = 0; + if (list_empty(&ns->ima_temp_rules)) return -EINVAL; + if (ns != &init_ima_ns) { + list_for_each_entry(entry, &ns->ima_temp_rules, list) + len1++; + list_for_each_entry(entry, &ns->ima_policy_rules, list)
Using list_for_each_entry() is fine here, because multiple policy updates at the same time are blocked. Please add a comment.
+ len2++; + if (len1 + len2 > 1024)
One variable should be enough.
quoted hunk ↗ jump to hunk
+ return -ENOSPC; + } return 0; }@@ -1848,7 +1861,7 @@ ssize_t ima_parse_add_rule(struct ima_namespace *ns, char *rule) if (*p == '#' || *p == '\0') return len; - entry = kzalloc(sizeof(*entry), GFP_KERNEL); + entry = kzalloc(sizeof(*entry), GFP_KERNEL_ACCOUNT); if (!entry) { integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL, op, "-ENOMEM", -ENOMEM, audit_info);
-- thanks, Mimi