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);
--
thanks,
Mimi
