| Message ID | 20180919075522.7684-4-nayna@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for architecture specific IMA policies | expand |
Hi Nayna,
Thank you for the patch! Perhaps something to improve:
url: https://github.com/0day-ci/linux/commits/Nayna-Jain/Add-support-for-architecture-specific-IMA-policies/20180920-035110
smatch warnings:
security/integrity/ima/ima_policy.c:489 add_rules() warn: should this be a bitwise op?
# https://github.com/0day-ci/linux/commit/84a2e186f940ebc6c34e6d276e55f665167a5bb8
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout 84a2e186f940ebc6c34e6d276e55f665167a5bb8
vim +489 security/integrity/ima/ima_policy.c
6f0911a6 Mimi Zohar 2018-04-12 477
84a2e186 Nayna Jain 2018-09-19 478 static void add_rules(struct ima_rule_entry *entries, int count,
84a2e186 Nayna Jain 2018-09-19 479 enum policy_rule_list file)
84a2e186 Nayna Jain 2018-09-19 480 {
84a2e186 Nayna Jain 2018-09-19 481 int i = 0;
84a2e186 Nayna Jain 2018-09-19 482
84a2e186 Nayna Jain 2018-09-19 483 for (i = 0; i < count; i++) {
84a2e186 Nayna Jain 2018-09-19 484 struct ima_rule_entry *entry;
84a2e186 Nayna Jain 2018-09-19 485
84a2e186 Nayna Jain 2018-09-19 486 if (file && IMA_DEFAULT_POLICY)
^^^^^^^^^^^^^^^^^^^^^^^^^^
84a2e186 Nayna Jain 2018-09-19 487 list_add_tail(&entries[i].list, &ima_default_rules);
84a2e186 Nayna Jain 2018-09-19 488
84a2e186 Nayna Jain 2018-09-19 @489 if (file && IMA_CUSTOM_POLICY) {
^^^^^^^^^^^^^^^^^^^^^^^^^
It does look like it should be "if (file & IMA_CUSTOM_POLICY) {" but I
haven't looked at the context besides what's here in this email.
84a2e186 Nayna Jain 2018-09-19 490 entry = kmemdup(&entries[i], sizeof(*entry),
84a2e186 Nayna Jain 2018-09-19 491 GFP_KERNEL);
84a2e186 Nayna Jain 2018-09-19 492 if (!entry)
84a2e186 Nayna Jain 2018-09-19 493 continue;
84a2e186 Nayna Jain 2018-09-19 494
84a2e186 Nayna Jain 2018-09-19 495 INIT_LIST_HEAD(&entry->list);
84a2e186 Nayna Jain 2018-09-19 496 list_add_tail(&entry->list, &ima_policy_rules);
84a2e186 Nayna Jain 2018-09-19 497 }
84a2e186 Nayna Jain 2018-09-19 498 if (entries[i].action == APPRAISE)
84a2e186 Nayna Jain 2018-09-19 499 temp_ima_appraise |= ima_appraise_flag(entries[i].func);
84a2e186 Nayna Jain 2018-09-19 500 if (entries[i].func == POLICY_CHECK)
84a2e186 Nayna Jain 2018-09-19 501 temp_ima_appraise |= IMA_APPRAISE_POLICY;
84a2e186 Nayna Jain 2018-09-19 502 }
84a2e186 Nayna Jain 2018-09-19 503 }
84a2e186 Nayna Jain 2018-09-19 504
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On 09/21/2018 02:04 PM, Dan Carpenter wrote: > Hi Nayna, > > Thank you for the patch! Perhaps something to improve: > > url: https://github.com/0day-ci/linux/commits/Nayna-Jain/Add-support-for-architecture-specific-IMA-policies/20180920-035110 > > smatch warnings: > security/integrity/ima/ima_policy.c:489 add_rules() warn: should this be a bitwise op? > > # https://github.com/0day-ci/linux/commit/84a2e186f940ebc6c34e6d276e55f665167a5bb8 > git remote add linux-review https://github.com/0day-ci/linux > git remote update linux-review > git checkout 84a2e186f940ebc6c34e6d276e55f665167a5bb8 > vim +489 security/integrity/ima/ima_policy.c > > 6f0911a6 Mimi Zohar 2018-04-12 477 > 84a2e186 Nayna Jain 2018-09-19 478 static void add_rules(struct ima_rule_entry *entries, int count, > 84a2e186 Nayna Jain 2018-09-19 479 enum policy_rule_list file) > 84a2e186 Nayna Jain 2018-09-19 480 { > 84a2e186 Nayna Jain 2018-09-19 481 int i = 0; > 84a2e186 Nayna Jain 2018-09-19 482 > 84a2e186 Nayna Jain 2018-09-19 483 for (i = 0; i < count; i++) { > 84a2e186 Nayna Jain 2018-09-19 484 struct ima_rule_entry *entry; > 84a2e186 Nayna Jain 2018-09-19 485 > 84a2e186 Nayna Jain 2018-09-19 486 if (file && IMA_DEFAULT_POLICY) > ^^^^^^^^^^^^^^^^^^^^^^^^^^ > 84a2e186 Nayna Jain 2018-09-19 487 list_add_tail(&entries[i].list, &ima_default_rules); > 84a2e186 Nayna Jain 2018-09-19 488 > 84a2e186 Nayna Jain 2018-09-19 @489 if (file && IMA_CUSTOM_POLICY) { > ^^^^^^^^^^^^^^^^^^^^^^^^^ > > It does look like it should be "if (file & IMA_CUSTOM_POLICY) {" but I > haven't looked at the context besides what's here in this email. Thanks Dan for noticing this. Yes, I will fix it and post the v4 version. Thanks & Regards, - Nayna > > 84a2e186 Nayna Jain 2018-09-19 490 entry = kmemdup(&entries[i], sizeof(*entry), > 84a2e186 Nayna Jain 2018-09-19 491 GFP_KERNEL); > 84a2e186 Nayna Jain 2018-09-19 492 if (!entry) > 84a2e186 Nayna Jain 2018-09-19 493 continue; > 84a2e186 Nayna Jain 2018-09-19 494 > 84a2e186 Nayna Jain 2018-09-19 495 INIT_LIST_HEAD(&entry->list); > 84a2e186 Nayna Jain 2018-09-19 496 list_add_tail(&entry->list, &ima_policy_rules); > 84a2e186 Nayna Jain 2018-09-19 497 } > 84a2e186 Nayna Jain 2018-09-19 498 if (entries[i].action == APPRAISE) > 84a2e186 Nayna Jain 2018-09-19 499 temp_ima_appraise |= ima_appraise_flag(entries[i].func); > 84a2e186 Nayna Jain 2018-09-19 500 if (entries[i].func == POLICY_CHECK) > 84a2e186 Nayna Jain 2018-09-19 501 temp_ima_appraise |= IMA_APPRAISE_POLICY; > 84a2e186 Nayna Jain 2018-09-19 502 } > 84a2e186 Nayna Jain 2018-09-19 503 } > 84a2e186 Nayna Jain 2018-09-19 504 > > --- > 0-DAY kernel test infrastructure Open Source Technology Center > https://lists.01.org/pipermail/kbuild-all Intel Corporation >
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 8c9499867c91..b58906a05736 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -58,6 +58,8 @@ enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE, enum policy_types { ORIGINAL_TCB = 1, DEFAULT_TCB }; +enum policy_rule_list { IMA_DEFAULT_POLICY = 1, IMA_CUSTOM_POLICY }; + struct ima_rule_entry { struct list_head list; int action; @@ -473,6 +475,33 @@ static int ima_appraise_flag(enum ima_hooks func) return 0; } +static void add_rules(struct ima_rule_entry *entries, int count, + enum policy_rule_list file) +{ + int i = 0; + + for (i = 0; i < count; i++) { + struct ima_rule_entry *entry; + + if (file && IMA_DEFAULT_POLICY) + list_add_tail(&entries[i].list, &ima_default_rules); + + if (file && IMA_CUSTOM_POLICY) { + entry = kmemdup(&entries[i], sizeof(*entry), + GFP_KERNEL); + if (!entry) + continue; + + INIT_LIST_HEAD(&entry->list); + list_add_tail(&entry->list, &ima_policy_rules); + } + if (entries[i].action == APPRAISE) + temp_ima_appraise |= ima_appraise_flag(entries[i].func); + if (entries[i].func == POLICY_CHECK) + temp_ima_appraise |= IMA_APPRAISE_POLICY; + } +} + /** * ima_init_policy - initialize the default measure rules. * @@ -481,28 +510,23 @@ static int ima_appraise_flag(enum ima_hooks func) */ void __init ima_init_policy(void) { - int i, measure_entries, appraise_entries, secure_boot_entries; - - /* if !ima_policy set entries = 0 so we load NO default rules */ - measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0; - appraise_entries = ima_use_appraise_tcb ? - ARRAY_SIZE(default_appraise_rules) : 0; - secure_boot_entries = ima_use_secure_boot ? - ARRAY_SIZE(secure_boot_rules) : 0; + int build_appraise_entries; - for (i = 0; i < measure_entries; i++) - list_add_tail(&dont_measure_rules[i].list, &ima_default_rules); + /* if !ima_policy, we load NO default rules */ + if (ima_policy) + add_rules(dont_measure_rules, ARRAY_SIZE(dont_measure_rules), + IMA_DEFAULT_POLICY); switch (ima_policy) { case ORIGINAL_TCB: - for (i = 0; i < ARRAY_SIZE(original_measurement_rules); i++) - list_add_tail(&original_measurement_rules[i].list, - &ima_default_rules); + add_rules(original_measurement_rules, + ARRAY_SIZE(original_measurement_rules), + IMA_DEFAULT_POLICY); break; case DEFAULT_TCB: - for (i = 0; i < ARRAY_SIZE(default_measurement_rules); i++) - list_add_tail(&default_measurement_rules[i].list, - &ima_default_rules); + add_rules(default_measurement_rules, + ARRAY_SIZE(default_measurement_rules), + IMA_DEFAULT_POLICY); default: break; } @@ -511,38 +535,30 @@ void __init ima_init_policy(void) * Insert the builtin "secure_boot" policy rules requiring file * signatures, prior to any other appraise rules. */ - for (i = 0; i < secure_boot_entries; i++) { - list_add_tail(&secure_boot_rules[i].list, &ima_default_rules); - temp_ima_appraise |= - ima_appraise_flag(secure_boot_rules[i].func); - } + if (ima_use_secure_boot) + add_rules(secure_boot_rules, ARRAY_SIZE(secure_boot_rules), + IMA_DEFAULT_POLICY); /* * Insert the build time appraise rules requiring file signatures * for both the initial and custom policies, prior to other appraise - * rules. + * rules. As the secure boot rules includes all of the build time + * rules, include either one or the other set of rules, but not both. */ - for (i = 0; i < ARRAY_SIZE(build_appraise_rules); i++) { - struct ima_rule_entry *entry; - - if (!secure_boot_entries) - list_add_tail(&build_appraise_rules[i].list, - &ima_default_rules); - - entry = kmemdup(&build_appraise_rules[i], sizeof(*entry), - GFP_KERNEL); - if (entry) - list_add_tail(&entry->list, &ima_policy_rules); - build_ima_appraise |= - ima_appraise_flag(build_appraise_rules[i].func); + build_appraise_entries = ARRAY_SIZE(build_appraise_rules); + if (build_appraise_entries) { + if (ima_use_secure_boot) + add_rules(build_appraise_rules, build_appraise_entries, + IMA_CUSTOM_POLICY); + else + add_rules(build_appraise_rules, build_appraise_entries, + IMA_DEFAULT_POLICY | IMA_CUSTOM_POLICY); } - for (i = 0; i < appraise_entries; i++) { - list_add_tail(&default_appraise_rules[i].list, - &ima_default_rules); - if (default_appraise_rules[i].func == POLICY_CHECK) - temp_ima_appraise |= IMA_APPRAISE_POLICY; - } + if (ima_use_appraise_tcb) + add_rules(default_appraise_rules, + ARRAY_SIZE(default_appraise_rules), + IMA_DEFAULT_POLICY); ima_rules = &ima_default_rules; ima_update_policy_flag();
This patch removes the code duplication in ima_init_policy() by defining a new function named add_rules(). Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com> --- security/integrity/ima/ima_policy.c | 98 +++++++++++++++++++++---------------- 1 file changed, 57 insertions(+), 41 deletions(-)