This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Paul Moore has reviewed the non-blocking LSM policy update backport, saying the SELinux part look good. The patchset was also reviewed by Mimi Zohar and everything looks ok.
I've tested the patches against the said issue and can confirm that the issue is fixed. I've also run the LTP testcases on the fixed IMA and all tests are passed.
This is a re-send of the original patchset as the original patchset might have a faulty cover letter. The original patchset could be found here: https://patchwork.kernel.org/project/linux-integrity/list/?series=709367
GUO Zihua (1): ima: Handle -ESTALE returned by ima_filter_rule_match()
Janne Karhunen (2): LSM: switch to blocking policy update notifiers ima: use the lsm policy update notifier
drivers/infiniband/core/device.c | 4 +- include/linux/security.h | 12 +-- security/integrity/ima/ima.h | 2 + security/integrity/ima/ima_main.c | 8 ++ security/integrity/ima/ima_policy.c | 151 ++++++++++++++++++++++------ security/security.c | 23 +++-- security/selinux/hooks.c | 2 +- security/selinux/selinuxfs.c | 2 +- 8 files changed, 155 insertions(+), 49 deletions(-)
From: Janne Karhunen janne.karhunen@gmail.com
[ Upstream commit 42df744c4166af6959eda2df1ee5cde744d4a1c3 ]
Atomic policy updaters are not very useful as they cannot usually perform the policy updates on their own. Since it seems that there is no strict need for the atomicity, switch to the blocking variant. While doing so, rename the functions accordingly.
Signed-off-by: Janne Karhunen janne.karhunen@gmail.com Acked-by: Paul Moore paul@paul-moore.com Acked-by: James Morris jamorris@linux.microsoft.com Signed-off-by: Mimi Zohar zohar@linux.ibm.com Signed-off-by: GUO Zihua guozihua@huawei.com --- drivers/infiniband/core/device.c | 4 ++-- include/linux/security.h | 12 ++++++------ security/security.c | 23 +++++++++++++---------- security/selinux/hooks.c | 2 +- security/selinux/selinuxfs.c | 2 +- 5 files changed, 23 insertions(+), 20 deletions(-)
diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c index ffd0f43e2129..ebcebb95f61b 100644 --- a/drivers/infiniband/core/device.c +++ b/drivers/infiniband/core/device.c @@ -1207,7 +1207,7 @@ static int __init ib_core_init(void) goto err_mad; }
- ret = register_lsm_notifier(&ibdev_lsm_nb); + ret = register_blocking_lsm_notifier(&ibdev_lsm_nb); if (ret) { pr_warn("Couldn't register LSM notifier. ret %d\n", ret); goto err_sa; @@ -1243,7 +1243,7 @@ static void __exit ib_core_cleanup(void) roce_gid_mgmt_cleanup(); nldev_exit(); rdma_nl_unregister(RDMA_NL_LS); - unregister_lsm_notifier(&ibdev_lsm_nb); + unregister_blocking_lsm_notifier(&ibdev_lsm_nb); ib_sa_cleanup(); ib_mad_cleanup(); addr_cleanup(); diff --git a/include/linux/security.h b/include/linux/security.h index 273877cf47bf..81e795c9c09a 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -191,9 +191,9 @@ struct security_mnt_opts { int num_mnt_opts; };
-int call_lsm_notifier(enum lsm_event event, void *data); -int register_lsm_notifier(struct notifier_block *nb); -int unregister_lsm_notifier(struct notifier_block *nb); +int call_blocking_lsm_notifier(enum lsm_event event, void *data); +int register_blocking_lsm_notifier(struct notifier_block *nb); +int unregister_blocking_lsm_notifier(struct notifier_block *nb);
static inline void security_init_mnt_opts(struct security_mnt_opts *opts) { @@ -409,17 +409,17 @@ int security_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen); struct security_mnt_opts { };
-static inline int call_lsm_notifier(enum lsm_event event, void *data) +static inline int call_blocking_lsm_notifier(enum lsm_event event, void *data) { return 0; }
-static inline int register_lsm_notifier(struct notifier_block *nb) +static inline int register_blocking_lsm_notifier(struct notifier_block *nb) { return 0; }
-static inline int unregister_lsm_notifier(struct notifier_block *nb) +static inline int unregister_blocking_lsm_notifier(struct notifier_block *nb) { return 0; } diff --git a/security/security.c b/security/security.c index fc1410550b79..4d20a15f18b4 100644 --- a/security/security.c +++ b/security/security.c @@ -38,7 +38,7 @@ #define SECURITY_NAME_MAX 10
struct security_hook_heads security_hook_heads __lsm_ro_after_init; -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain); +static BLOCKING_NOTIFIER_HEAD(blocking_lsm_notifier_chain);
char *lsm_names; /* Boot-time LSM user choice */ @@ -180,23 +180,26 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count, panic("%s - Cannot get early memory.\n", __func__); }
-int call_lsm_notifier(enum lsm_event event, void *data) +int call_blocking_lsm_notifier(enum lsm_event event, void *data) { - return atomic_notifier_call_chain(&lsm_notifier_chain, event, data); + return blocking_notifier_call_chain(&blocking_lsm_notifier_chain, + event, data); } -EXPORT_SYMBOL(call_lsm_notifier); +EXPORT_SYMBOL(call_blocking_lsm_notifier);
-int register_lsm_notifier(struct notifier_block *nb) +int register_blocking_lsm_notifier(struct notifier_block *nb) { - return atomic_notifier_chain_register(&lsm_notifier_chain, nb); + return blocking_notifier_chain_register(&blocking_lsm_notifier_chain, + nb); } -EXPORT_SYMBOL(register_lsm_notifier); +EXPORT_SYMBOL(register_blocking_lsm_notifier);
-int unregister_lsm_notifier(struct notifier_block *nb) +int unregister_blocking_lsm_notifier(struct notifier_block *nb) { - return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb); + return blocking_notifier_chain_unregister(&blocking_lsm_notifier_chain, + nb); } -EXPORT_SYMBOL(unregister_lsm_notifier); +EXPORT_SYMBOL(unregister_blocking_lsm_notifier);
/* * Hook list operation macros. diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 41e24df986eb..77f2147709b1 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -199,7 +199,7 @@ static int selinux_lsm_notifier_avc_callback(u32 event) { if (event == AVC_CALLBACK_RESET) { sel_ib_pkey_flush(); - call_lsm_notifier(LSM_POLICY_CHANGE, NULL); + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL); }
return 0; diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c index 60b3f16bb5c7..4f72d0998580 100644 --- a/security/selinux/selinuxfs.c +++ b/security/selinux/selinuxfs.c @@ -180,7 +180,7 @@ static ssize_t sel_write_enforce(struct file *file, const char __user *buf, selnl_notify_setenforce(new_value); selinux_status_update_setenforce(state, new_value); if (!new_value) - call_lsm_notifier(LSM_POLICY_CHANGE, NULL); + call_blocking_lsm_notifier(LSM_POLICY_CHANGE, NULL); } length = count; out:
From: Janne Karhunen janne.karhunen@gmail.com
[ Upstream commit b169424551930a9325f700f502802f4d515194e5 ]
Don't do lazy policy updates while running the rule matching, run the updates as they happen.
Depends on commit f242064c5df3 ("LSM: switch to blocking policy update notifiers")
Signed-off-by: Janne Karhunen janne.karhunen@gmail.com Signed-off-by: Mimi Zohar zohar@linux.ibm.com Signed-off-by: GUO Zihua guozihua@huawei.com --- security/integrity/ima/ima.h | 2 + security/integrity/ima/ima_main.c | 8 ++ security/integrity/ima/ima_policy.c | 118 ++++++++++++++++++++++------ 3 files changed, 105 insertions(+), 23 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h index e2916b115b93..dc564ed9a790 100644 --- a/security/integrity/ima/ima.h +++ b/security/integrity/ima/ima.h @@ -154,6 +154,8 @@ int ima_measurements_show(struct seq_file *m, void *v); unsigned long ima_get_binary_runtime_size(void); int ima_init_template(void); void ima_init_template_list(void); +int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, + void *lsm_data);
/* * used to protect h_table and sha_table diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 2d31921fbda4..55681872c6ce 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -41,6 +41,10 @@ int ima_appraise; int ima_hash_algo = HASH_ALGO_SHA1; static int hash_setup_done;
+static struct notifier_block ima_lsm_policy_notifier = { + .notifier_call = ima_lsm_policy_change, +}; + static int __init hash_setup(char *str) { struct ima_template_desc *template_desc = ima_template_desc_current(); @@ -553,6 +557,10 @@ static int __init init_ima(void) error = ima_init(); }
+ error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier); + if (error) + pr_warn("Couldn't register LSM notifier, error %d\n", error); + if (!error) ima_update_policy_flag();
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index b2dadff3626b..5a15524bca4c 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -241,46 +241,124 @@ static int __init default_appraise_policy_setup(char *str) } __setup("ima_appraise_tcb", default_appraise_policy_setup);
-static void ima_free_rule(struct ima_rule_entry *entry) +static void ima_lsm_free_rule(struct ima_rule_entry *entry) { int i;
+ for (i = 0; i < MAX_LSM_RULES; i++) { + security_filter_rule_free(entry->lsm[i].rule); + kfree(entry->lsm[i].args_p); + } +} + +static void ima_free_rule(struct ima_rule_entry *entry) +{ if (!entry) return;
kfree(entry->fsname); + ima_lsm_free_rule(entry); + kfree(entry); +} + +static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry) +{ + struct ima_rule_entry *nentry; + int i, result; + + nentry = kmalloc(sizeof(*nentry), GFP_KERNEL); + if (!nentry) + return NULL; + + /* + * Immutable elements are copied over as pointers and data; only + * lsm rules can change + */ + memcpy(nentry, entry, sizeof(*nentry)); + memset(nentry->lsm, 0, FIELD_SIZEOF(struct ima_rule_entry, lsm)); + for (i = 0; i < MAX_LSM_RULES; i++) { - security_filter_rule_free(entry->lsm[i].rule); - kfree(entry->lsm[i].args_p); + if (!entry->lsm[i].rule) + continue; + + nentry->lsm[i].type = entry->lsm[i].type; + nentry->lsm[i].args_p = kstrdup(entry->lsm[i].args_p, + GFP_KERNEL); + if (!nentry->lsm[i].args_p) + goto out_err; + + result = security_filter_rule_init(nentry->lsm[i].type, + Audit_equal, + nentry->lsm[i].args_p, + &nentry->lsm[i].rule); + if (result == -EINVAL) + pr_warn("ima: rule for LSM '%d' is undefined\n", + entry->lsm[i].type); } + return nentry; + +out_err: + ima_lsm_free_rule(nentry); + kfree(nentry); + return NULL; +} + +static int ima_lsm_update_rule(struct ima_rule_entry *entry) +{ + struct ima_rule_entry *nentry; + + nentry = ima_lsm_copy_rule(entry); + if (!nentry) + return -ENOMEM; + + list_replace_rcu(&entry->list, &nentry->list); + synchronize_rcu(); + ima_lsm_free_rule(entry); kfree(entry); + + return 0; }
/* * The LSM policy can be reloaded, leaving the IMA LSM based rules referring * to the old, stale LSM policy. Update the IMA LSM based rules to reflect - * the reloaded LSM policy. We assume the rules still exist; and BUG_ON() if - * they don't. + * the reloaded LSM policy. */ static void ima_lsm_update_rules(void) { - struct ima_rule_entry *entry; - int result; - int i; + struct ima_rule_entry *entry, *e; + int i, result, needs_update;
- list_for_each_entry(entry, &ima_policy_rules, list) { + list_for_each_entry_safe(entry, e, &ima_policy_rules, list) { + needs_update = 0; for (i = 0; i < MAX_LSM_RULES; i++) { - if (!entry->lsm[i].rule) - continue; - result = security_filter_rule_init(entry->lsm[i].type, - Audit_equal, - entry->lsm[i].args_p, - &entry->lsm[i].rule); - BUG_ON(!entry->lsm[i].rule); + if (entry->lsm[i].rule) { + needs_update = 1; + break; + } + } + if (!needs_update) + continue; + + result = ima_lsm_update_rule(entry); + if (result) { + pr_err("ima: lsm rule update error %d\n", + result); + return; } } }
+int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event, + void *lsm_data) +{ + if (event != LSM_POLICY_CHANGE) + return NOTIFY_DONE; + + ima_lsm_update_rules(); + return NOTIFY_OK; +} + /** * ima_match_rules - determine whether an inode matches the measure rule. * @rule: a pointer to a rule @@ -334,11 +412,10 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, for (i = 0; i < MAX_LSM_RULES; i++) { int rc = 0; u32 osid; - int retried = 0;
if (!rule->lsm[i].rule) continue; -retry: + switch (i) { case LSM_OBJ_USER: case LSM_OBJ_ROLE: @@ -361,11 +438,6 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, default: break; } - if ((rc < 0) && (!retried)) { - retried = 1; - ima_lsm_update_rules(); - goto retry; - } if (!rc) return false; }
[ Upstream commit c7423dbdbc9ecef7fff5239d144cad4b9887f4de ]
IMA relies on the blocking LSM policy notifier callback to update the LSM based IMA policy rules.
When SELinux update its policies, IMA would be notified and starts updating all its lsm rules one-by-one. During this time, -ESTALE would be returned by ima_filter_rule_match() if it is called with a LSM rule that has not yet been updated. In ima_match_rules(), -ESTALE is not handled, and the LSM rule is considered a match, causing extra files to be measured by IMA.
Fix it by re-initializing a temporary rule if -ESTALE is returned by ima_filter_rule_match(). The origin rule in the rule list would be updated by the LSM policy notifier callback.
Fixes: b16942455193 ("ima: use the lsm policy update notifier") Signed-off-by: GUO Zihua guozihua@huawei.com Reviewed-by: Roberto Sassu roberto.sassu@huawei.com Signed-off-by: Mimi Zohar zohar@linux.ibm.com Signed-off-by: GUO Zihua guozihua@huawei.com --- security/integrity/ima/ima_policy.c | 37 ++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 8 deletions(-)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c index 5a15524bca4c..9d500a06c710 100644 --- a/security/integrity/ima/ima_policy.c +++ b/security/integrity/ima/ima_policy.c @@ -375,6 +375,9 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, enum ima_hooks func, int mask) { int i; + bool result = false; + struct ima_rule_entry *lsm_rule = rule; + bool rule_reinitialized = false;
if ((rule->flags & IMA_FUNC) && (rule->func != func && func != POST_SETATTR)) @@ -413,35 +416,53 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode, int rc = 0; u32 osid;
- if (!rule->lsm[i].rule) + if (!lsm_rule->lsm[i].rule) continue;
+retry: switch (i) { case LSM_OBJ_USER: case LSM_OBJ_ROLE: case LSM_OBJ_TYPE: security_inode_getsecid(inode, &osid); rc = security_filter_rule_match(osid, - rule->lsm[i].type, + lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule, + lsm_rule->lsm[i].rule, NULL); break; case LSM_SUBJ_USER: case LSM_SUBJ_ROLE: case LSM_SUBJ_TYPE: rc = security_filter_rule_match(secid, - rule->lsm[i].type, + lsm_rule->lsm[i].type, Audit_equal, - rule->lsm[i].rule, + lsm_rule->lsm[i].rule, NULL); default: break; } - if (!rc) - return false; + + if (rc == -ESTALE && !rule_reinitialized) { + lsm_rule = ima_lsm_copy_rule(rule); + if (lsm_rule) { + rule_reinitialized = true; + goto retry; + } + } + if (!rc) { + result = false; + goto out; + } + } + result = true; + +out: + if (rule_reinitialized) { + ima_lsm_free_rule(lsm_rule); + kfree(lsm_rule); } - return true; + return result; }
/*
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
On 2023/2/3 1:20, Sasha Levin wrote:
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
Hi Sasha.
The issue mentioned in this patch was fixed already in the newer kernel. All three patches here are backports from mainline.
On Fri, Feb 03, 2023 at 09:10:13AM +0800, Guozihua (Scott) wrote:
On 2023/2/3 1:20, Sasha Levin wrote:
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
Hi Sasha.
The issue mentioned in this patch was fixed already in the newer kernel. All three patches here are backports from mainline.
Ok, now queued up, thanks.
greg k-h
On Fri, Feb 03, 2023 at 08:44:51AM +0100, Greg KH wrote:
On Fri, Feb 03, 2023 at 09:10:13AM +0800, Guozihua (Scott) wrote:
On 2023/2/3 1:20, Sasha Levin wrote:
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
Hi Sasha.
The issue mentioned in this patch was fixed already in the newer kernel. All three patches here are backports from mainline.
Ok, now queued up, thanks.
Nope, I've now dropped them all as you did not include the needed fix up commits as well. We can not add patches to the stable tree that are known broken, right?
How well did you test this? I see at least 3 missing patches that you should have had in this patch series for it to work properly.
greg k-h
On Fri, Feb 03, 2023 at 08:49:05AM +0100, Greg KH wrote:
On Fri, Feb 03, 2023 at 08:44:51AM +0100, Greg KH wrote:
On Fri, Feb 03, 2023 at 09:10:13AM +0800, Guozihua (Scott) wrote:
On 2023/2/3 1:20, Sasha Levin wrote:
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
Hi Sasha.
The issue mentioned in this patch was fixed already in the newer kernel. All three patches here are backports from mainline.
Ok, now queued up, thanks.
Nope, I've now dropped them all as you did not include the needed fix up commits as well. We can not add patches to the stable tree that are known broken, right?
How well did you test this? I see at least 3 missing patches that you should have had in this patch series for it to work properly.
Ah, you didn't even test this series, as it breaks the build as-submitted.
{sigh}
In order for us to take this, I think you need to find someone else who will validate your patch series _FIRST_ before submitting it to us. And I want their tested-by on them validating that it did actually work (if for no other reason than to have someone else be willing to be responsible if things go bad.)
Breaking our builds and forcing me to point out missing patches is not how the stable kernel process works in any sane manner.
greg k-h
On 2023/2/3 15:54, Greg KH wrote:
On Fri, Feb 03, 2023 at 08:49:05AM +0100, Greg KH wrote:
On Fri, Feb 03, 2023 at 08:44:51AM +0100, Greg KH wrote:
On Fri, Feb 03, 2023 at 09:10:13AM +0800, Guozihua (Scott) wrote:
On 2023/2/3 1:20, Sasha Levin wrote:
On Wed, Feb 01, 2023 at 10:39:49AM +0800, GUO Zihua wrote:
This series backports patches in order to resolve the issue discussed here: https://lore.kernel.org/selinux/389334fe-6e12-96b2-6ce9-9f0e8fcb85bf@huawei....
This required backporting the non-blocking LSM policy update mechanism prerequisite patches.
Do we not need this on newer kernels? Why only 4.19?
Hi Sasha.
The issue mentioned in this patch was fixed already in the newer kernel. All three patches here are backports from mainline.
Ok, now queued up, thanks.
Nope, I've now dropped them all as you did not include the needed fix up commits as well. We can not add patches to the stable tree that are known broken, right?
How well did you test this? I see at least 3 missing patches that you should have had in this patch series for it to work properly.
Ah, you didn't even test this series, as it breaks the build as-submitted.
{sigh}
In order for us to take this, I think you need to find someone else who will validate your patch series _FIRST_ before submitting it to us. And I want their tested-by on them validating that it did actually work (if for no other reason than to have someone else be willing to be responsible if things go bad.)
Breaking our builds and forcing me to point out missing patches is not how the stable kernel process works in any sane manner.
greg k-h
Sorry for the burden. Still trying to work out how things are done here.
It seems that when I test it out, it did not build with the allmodconfig which would report an error. And by the "fixes up commit" I supposed it mean the commits with the "Fixes" tag points to the three commits I submitted.
I'll submit a new patch set soon, which would include the following fixes commits.
Again, sorry for the burden.
linux-stable-mirror@lists.linaro.org
-
Greg KH -
GUO Zihua -
Guozihua (Scott)
-
Sasha Levin