| Message ID | 20220201203735.164593-27-stefanb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ima: Namespace IMA with audit support in IMA-ns | expand |
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@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. > > 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);
On 2/23/22 10:38, Mimi Zohar wrote: > 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? There's typically no cgroup for the int_ima_ns, so not effect on init_ima_ns. Here's the updated text: Limit the number of policy rules one can set in non-init_ima_ns to a hardcoded 1024 rules to restrict the amount of memory used for IMA's policy. Ignore the added rules if the user attempts to exceed this limit by setting too many additional rules. Switch the accounting for the memory allocated for IMA policy rules to GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect. This switch has no effect on the init_ima_ns. Stefan >> 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. > >> >> 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);
On Wed, 2022-02-23 at 11:37 -0500, Stefan Berger wrote: > On 2/23/22 10:38, Mimi Zohar wrote: > > 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? > > There's typically no cgroup for the int_ima_ns, so not effect on > init_ima_ns. > > Here's the updated text: > > Limit the number of policy rules one can set in non-init_ima_ns to a > hardcoded 1024 rules to restrict the amount of memory used for IMA's > policy. The question is "why" there should be a difference between the init_ima_ns and non-init_ima_ns cases. Perhaps something like, "Only host root with CAP_SYS_ADMIN may write init_ima_ns policy rules, but in the non-init_ima_ns case root in the namespace with CAP_MAC_ADMIN privileges may write IMA policy rules. Limit the total number of IMA policy rules per namespace." > Ignore the added rules if the user attempts to exceed this > limit by setting too many additional rules. > > Switch the accounting for the memory allocated for IMA policy rules to > GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect. > This switch has no effect on the init_ima_ns. thanks, Mimi
On 2/23/22 12:04, Mimi Zohar wrote: > On Wed, 2022-02-23 at 11:37 -0500, Stefan Berger wrote: >> On 2/23/22 10:38, Mimi Zohar wrote: >>> 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? >> There's typically no cgroup for the int_ima_ns, so not effect on >> init_ima_ns. >> >> Here's the updated text: >> >> Limit the number of policy rules one can set in non-init_ima_ns to a >> hardcoded 1024 rules to restrict the amount of memory used for IMA's >> policy. > The question is "why" there should be a difference between the > init_ima_ns and non-init_ima_ns cases. Perhaps something like, "Only Chistian had suggested it to not have the number of policy rules unbounded. > host root with CAP_SYS_ADMIN may write init_ima_ns policy rules, but in > the non-init_ima_ns case root in the namespace with CAP_MAC_ADMIN > privileges may write IMA policy rules. Limit the total number of IMA > policy rules per namespace." What does it have to do with CAP_SYS_ADMIN and CAP_MAC_ADMIN and why mention these here? It seem primarily a limit of number of rules to avoid huge kernel memory consumption in the case that a cgroup limit for memory was not set up. > >> Ignore the added rules if the user attempts to exceed this >> limit by setting too many additional rules. >> >> Switch the accounting for the memory allocated for IMA policy rules to >> GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect. >> This switch has no effect on the init_ima_ns. > thanks, > > Mimi >
On Wed, 2022-02-23 at 15:45 -0500, Stefan Berger wrote: > avoid huge kernel memory consumption in the case that a cgroup limit for > memory was not set up. Ok, that is the motivation for the this patch.
On 2/23/22 15:59, Mimi Zohar wrote: > On Wed, 2022-02-23 at 15:45 -0500, Stefan Berger wrote: > >> avoid huge kernel memory consumption in the case that a cgroup limit for >> memory was not set up. > Ok, that is the motivation for the this patch. > Any user can create several user namespaces and with that several IMA namespaces and now we want to limit the number of rules inside an IMA namespace to limit the amount of kernel memory the policy rules are consuming. It isn't necessarily related to cgroups but a hard limit on the number of rules to avoid wasted of memory.
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) + 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); 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) + len2++; + if (len1 + len2 > 1024) + 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);
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. Switch the accounting for the memory allocated for IMA policy rules to GFP_KERNEL_ACCOUNT so that cgroups kernel memory accounting takes effect. 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(-)