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?
Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
---
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.
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.
+ 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);
