All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] LSM: switch to blocking policy update notifiers
@ 2019-06-05  8:36 Janne Karhunen
  2019-06-05  8:36 ` [PATCH 2/2] ima: use the lsm policy update notifier Janne Karhunen
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Janne Karhunen @ 2019-06-05  8:36 UTC (permalink / raw)
  To: sds, zohar, paul; +Cc: linux-integrity, linux-security-module, Janne Karhunen

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>
---
 drivers/infiniband/core/device.c |  6 +++---
 include/linux/security.h         |  6 +++---
 security/security.c              | 23 +++++++++++++----------
 security/selinux/hooks.c         |  2 +-
 security/selinux/selinuxfs.c     |  2 +-
 5 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 78dc07c6ac4b..61c0c93a2e73 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -2499,7 +2499,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;
@@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
 	return 0;
 
 err_compat:
-	unregister_lsm_notifier(&ibdev_lsm_nb);
+	unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
 err_sa:
 	ib_sa_cleanup();
 err_mad:
@@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
 	nldev_exit();
 	rdma_nl_unregister(RDMA_NL_LS);
 	unregister_pernet_device(&rdma_dev_net_ops);
-	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 659071c2e57c..fc655fbe44ad 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
 
 #ifdef CONFIG_SECURITY
 
-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);
 
 /* prototypes */
 extern int security_init(void);
diff --git a/security/security.c b/security/security.c
index c01a88f65ad8..6bfc7636ddb7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -39,7 +39,7 @@
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
 
 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);
 
 static struct kmem_cache *lsm_file_cache;
 static struct kmem_cache *lsm_inode_cache;
@@ -430,23 +430,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);
 
 /**
  * lsm_cred_alloc - allocate a composite cred blob
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c61787b15f27..c1e37018c8eb 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -197,7 +197,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 145ee62f205a..1e2e3e4b5fdb 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:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/2] ima: use the lsm policy update notifier
  2019-06-05  8:36 [PATCH 1/2] LSM: switch to blocking policy update notifiers Janne Karhunen
@ 2019-06-05  8:36 ` Janne Karhunen
  2019-06-06 21:59   ` Mimi Zohar
  2019-06-05 15:23 ` [PATCH 1/2] LSM: switch to blocking policy update notifiers Casey Schaufler
  2019-06-05 19:15 ` Paul Moore
  2 siblings, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-06-05  8:36 UTC (permalink / raw)
  To: sds, zohar, paul; +Cc: linux-integrity, linux-security-module, Janne Karhunen

Don't do lazy policy updates while running the rule matching,
run the updates as they happen.

Depends on commit 2d1d5cee66d1 ("LSM: switch to blocking policy update notifiers")

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/integrity/ima/ima.h        |   2 +
 security/integrity/ima/ima_main.c   |   8 ++
 security/integrity/ima/ima_policy.c | 124 +++++++++++++++++++++++-----
 3 files changed, 114 insertions(+), 20 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..2203451862d4 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
 int ima_init_template(void);
 void ima_init_template_list(void);
 int __init ima_init_digests(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 f16353b5097e..9e3ea8a3f2db 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -43,6 +43,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();
@@ -621,6 +625,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 1cc822a59054..7129dc4cd396 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -249,31 +249,121 @@ static int __init default_appraise_policy_setup(char *str)
 }
 __setup("ima_appraise_tcb", default_appraise_policy_setup);
 
+static void ima_lsm_free_rule(struct ima_rule_entry *entry)
+{
+	int i;
+
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		kfree(entry->lsm[i].rule);
+		kfree(entry->lsm[i].args_p);
+	}
+	kfree(entry->fsname);
+	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;
+
+	memcpy(nentry, entry, sizeof(*nentry));
+	nentry->fsname = NULL;
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		nentry->lsm[i].rule = NULL;
+		nentry->lsm[i].args_p = NULL;
+	}
+
+	if (entry->fsname) {
+		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
+		if (!nentry->fsname)
+			goto out_err;
+	}
+	for (i = 0; i < MAX_LSM_RULES; i++) {
+		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 invalid\n",
+				entry->lsm[i].type);
+
+	}
+	return nentry;
+
+out_err:
+	ima_lsm_free_rule(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);
+
+	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
@@ -327,11 +417,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:
@@ -352,11 +441,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;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05  8:36 [PATCH 1/2] LSM: switch to blocking policy update notifiers Janne Karhunen
  2019-06-05  8:36 ` [PATCH 2/2] ima: use the lsm policy update notifier Janne Karhunen
@ 2019-06-05 15:23 ` Casey Schaufler
  2019-06-05 16:51   ` Janne Karhunen
  2019-06-05 19:15 ` Paul Moore
  2 siblings, 1 reply; 15+ messages in thread
From: Casey Schaufler @ 2019-06-05 15:23 UTC (permalink / raw)
  To: Janne Karhunen, sds, zohar, paul; +Cc: linux-integrity, linux-security-module


[-- Attachment #1.1: Type: text/plain, Size: 5714 bytes --]

On 6/5/2019 1:36 AM, Janne Karhunen wrote:
> 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>
> ---
>  drivers/infiniband/core/device.c |  6 +++---
>  include/linux/security.h         |  6 +++---
>  security/security.c              | 23 +++++++++++++----------
>  security/selinux/hooks.c         |  2 +-
>  security/selinux/selinuxfs.c     |  2 +-
>  5 files changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 78dc07c6ac4b..61c0c93a2e73 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2499,7 +2499,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;
> @@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
>  	return 0;
>  
>  err_compat:
> -	unregister_lsm_notifier(&ibdev_lsm_nb);
> +	unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
>  err_sa:
>  	ib_sa_cleanup();
>  err_mad:
> @@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
>  	nldev_exit();
>  	rdma_nl_unregister(RDMA_NL_LS);
>  	unregister_pernet_device(&rdma_dev_net_ops);
> -	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 659071c2e57c..fc655fbe44ad 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>  
>  #ifdef CONFIG_SECURITY
>  
> -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);

Why is it important to change the names of these hooks?
It's not like you had call_atomic_lsm_notifier() before.
It seems like a lot of unnecessary code churn.

>  
>  /* prototypes */
>  extern int security_init(void);
> diff --git a/security/security.c b/security/security.c
> index c01a88f65ad8..6bfc7636ddb7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>  
>  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);
>  
>  static struct kmem_cache *lsm_file_cache;
>  static struct kmem_cache *lsm_inode_cache;
> @@ -430,23 +430,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);
>  
>  /**
>   * lsm_cred_alloc - allocate a composite cred blob
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..c1e37018c8eb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -197,7 +197,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 145ee62f205a..1e2e3e4b5fdb 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:


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05 15:23 ` [PATCH 1/2] LSM: switch to blocking policy update notifiers Casey Schaufler
@ 2019-06-05 16:51   ` Janne Karhunen
  2019-06-05 17:05     ` Casey Schaufler
  0 siblings, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-06-05 16:51 UTC (permalink / raw)
  To: Casey Schaufler
  Cc: Stephen Smalley, Mimi Zohar, Paul Moore, linux-integrity,
	linux-security-module

On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:

> > -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);
>
> Why is it important to change the names of these hooks?
> It's not like you had call_atomic_lsm_notifier() before.
> It seems like a lot of unnecessary code churn.

Paul was thinking there will eventually be two sets of notifiers
(atomic and blocking) and this creates the clear separation. That's
probably true, but it does indeed create a pretty big change that it
is not really needed yet. I'm fine either way.


--
Janne

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05 16:51   ` Janne Karhunen
@ 2019-06-05 17:05     ` Casey Schaufler
  2019-06-05 19:14       ` Paul Moore
  0 siblings, 1 reply; 15+ messages in thread
From: Casey Schaufler @ 2019-06-05 17:05 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Stephen Smalley, Mimi Zohar, Paul Moore, linux-integrity,
	linux-security-module, casey

On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
>>> -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);
>> Why is it important to change the names of these hooks?
>> It's not like you had call_atomic_lsm_notifier() before.
>> It seems like a lot of unnecessary code churn.
> Paul was thinking there will eventually be two sets of notifiers
> (atomic and blocking) and this creates the clear separation.

One hook with an added "bool blocking" argument, if
that's the only difference?

>  That's
> probably true, but it does indeed create a pretty big change that it
> is not really needed yet. I'm fine either way.
>
>
> --
> Janne

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05 17:05     ` Casey Schaufler
@ 2019-06-05 19:14       ` Paul Moore
  2019-06-07  0:45         ` James Morris
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Moore @ 2019-06-05 19:14 UTC (permalink / raw)
  To: Casey Schaufler, Janne Karhunen
  Cc: Stephen Smalley, Mimi Zohar, linux-integrity, linux-security-module

On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> > On Wed, Jun 5, 2019 at 6:23 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >
> >>> -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);
> >> Why is it important to change the names of these hooks?
> >> It's not like you had call_atomic_lsm_notifier() before.
> >> It seems like a lot of unnecessary code churn.
> > Paul was thinking there will eventually be two sets of notifiers
> > (atomic and blocking) and this creates the clear separation.
>
> One hook with an added "bool blocking" argument, if
> that's the only difference?

I think there is value in keeping a similar convention to the notifier
code on which this is based, see include/linux/notifier.h.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05  8:36 [PATCH 1/2] LSM: switch to blocking policy update notifiers Janne Karhunen
  2019-06-05  8:36 ` [PATCH 2/2] ima: use the lsm policy update notifier Janne Karhunen
  2019-06-05 15:23 ` [PATCH 1/2] LSM: switch to blocking policy update notifiers Casey Schaufler
@ 2019-06-05 19:15 ` Paul Moore
  2 siblings, 0 replies; 15+ messages in thread
From: Paul Moore @ 2019-06-05 19:15 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Stephen Smalley, zohar, linux-integrity, linux-security-module

On Wed, Jun 5, 2019 at 4:36 AM Janne Karhunen <janne.karhunen@gmail.com> wrote:
>
> 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>
> ---
>  drivers/infiniband/core/device.c |  6 +++---
>  include/linux/security.h         |  6 +++---
>  security/security.c              | 23 +++++++++++++----------
>  security/selinux/hooks.c         |  2 +-
>  security/selinux/selinuxfs.c     |  2 +-
>  5 files changed, 21 insertions(+), 18 deletions(-)

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 78dc07c6ac4b..61c0c93a2e73 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -2499,7 +2499,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;
> @@ -2518,7 +2518,7 @@ static int __init ib_core_init(void)
>         return 0;
>
>  err_compat:
> -       unregister_lsm_notifier(&ibdev_lsm_nb);
> +       unregister_blocking_lsm_notifier(&ibdev_lsm_nb);
>  err_sa:
>         ib_sa_cleanup();
>  err_mad:
> @@ -2544,7 +2544,7 @@ static void __exit ib_core_cleanup(void)
>         nldev_exit();
>         rdma_nl_unregister(RDMA_NL_LS);
>         unregister_pernet_device(&rdma_dev_net_ops);
> -       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 659071c2e57c..fc655fbe44ad 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -189,9 +189,9 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
>
>  #ifdef CONFIG_SECURITY
>
> -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);
>
>  /* prototypes */
>  extern int security_init(void);
> diff --git a/security/security.c b/security/security.c
> index c01a88f65ad8..6bfc7636ddb7 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
>  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);
>
>  static struct kmem_cache *lsm_file_cache;
>  static struct kmem_cache *lsm_inode_cache;
> @@ -430,23 +430,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);
>
>  /**
>   * lsm_cred_alloc - allocate a composite cred blob
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index c61787b15f27..c1e37018c8eb 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -197,7 +197,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 145ee62f205a..1e2e3e4b5fdb 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:
> --
> 2.17.1
>

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] ima: use the lsm policy update notifier
  2019-06-05  8:36 ` [PATCH 2/2] ima: use the lsm policy update notifier Janne Karhunen
@ 2019-06-06 21:59   ` Mimi Zohar
  2019-06-06 22:28     ` Mimi Zohar
  0 siblings, 1 reply; 15+ messages in thread
From: Mimi Zohar @ 2019-06-06 21:59 UTC (permalink / raw)
  To: Janne Karhunen, sds, paul; +Cc: linux-integrity, linux-security-module

Hi Janne,

On Wed, 2019-06-05 at 11:36 +0300, Janne Karhunen wrote:
> Don't do lazy policy updates while running the rule matching,
> run the updates as they happen.
> 
> Depends on commit 2d1d5cee66d1 ("LSM: switch to blocking policy update notifiers")
> 
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>

Thanks!  Just a couple of minor things.  Comments inline below.

> ---
>  security/integrity/ima/ima.h        |   2 +
>  security/integrity/ima/ima_main.c   |   8 ++
>  security/integrity/ima/ima_policy.c | 124 +++++++++++++++++++++++-----
>  3 files changed, 114 insertions(+), 20 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index d213e835c498..2203451862d4 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -154,6 +154,8 @@ unsigned long ima_get_binary_runtime_size(void);
>  int ima_init_template(void);
>  void ima_init_template_list(void);
>  int __init ima_init_digests(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 f16353b5097e..9e3ea8a3f2db 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -43,6 +43,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();
> @@ -621,6 +625,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 1cc822a59054..7129dc4cd396 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -249,31 +249,121 @@ static int __init default_appraise_policy_setup(char *str)
>  }
>  __setup("ima_appraise_tcb", default_appraise_policy_setup);
>  
> +static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> +{
> +	int i;
> +
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		kfree(entry->lsm[i].rule);
> +		kfree(entry->lsm[i].args_p);
> +	}
> +	kfree(entry->fsname);
> +	kfree(entry);
> +}

Matthew's patch, which adds per policy template format support, adds a
"template" field to entry.  In case anyone wants to backport this
patch, it might be simpler to make the change as a separate patch.


> +
> +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;
> +
> +	memcpy(nentry, entry, sizeof(*nentry));
> +	nentry->fsname = NULL;
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		nentry->lsm[i].rule = NULL;
> +		nentry->lsm[i].args_p = NULL;
> +	}
> +
> +	if (entry->fsname) {
> +		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
> +		if (!nentry->fsname)
> +			goto out_err;
> +	}
> +	for (i = 0; i < MAX_LSM_RULES; i++) {
> +		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 invalid\n",
> +				entry->lsm[i].type);

If LSM labels can come and go, then perhaps instead of saying
"invalid" say "undefined" or "missing".


> +
> +	}
> +	return nentry;
> +
> +out_err:
> +	ima_lsm_free_rule(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);
> +
> +	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);

No need for separate line.

Mimi

> +			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
> @@ -327,11 +417,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:
> @@ -352,11 +441,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;
>  	}


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/2] ima: use the lsm policy update notifier
  2019-06-06 21:59   ` Mimi Zohar
@ 2019-06-06 22:28     ` Mimi Zohar
  0 siblings, 0 replies; 15+ messages in thread
From: Mimi Zohar @ 2019-06-06 22:28 UTC (permalink / raw)
  To: Janne Karhunen, sds, paul; +Cc: linux-integrity, linux-security-module

Hi Janne,

One more comment below ...

> > +
> > +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;
> > +
> > +	memcpy(nentry, entry, sizeof(*nentry));
> > +	nentry->fsname = NULL;
> > +	for (i = 0; i < MAX_LSM_RULES; i++) {
> > +		nentry->lsm[i].rule = NULL;
> > +		nentry->lsm[i].args_p = NULL;
> > +	}

I don't think this loop is necessary.  Either use kzalloc() or move
the initialization to inside the loop below.

> > +
> > +	if (entry->fsname) {
> > +		nentry->fsname = kstrdup(entry->fsname, GFP_KERNEL);
> > +		if (!nentry->fsname)
> > +			goto out_err;
> > +	}
> > +	for (i = 0; i < MAX_LSM_RULES; i++) {
> > +		if (!entry->lsm[i].rule)
> > +			continue;

To here.

> > +
> > +		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;

If the memory allocation fails, then nentry will be freed anyway.

thanks,

Mimid


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-05 19:14       ` Paul Moore
@ 2019-06-07  0:45         ` James Morris
  2019-06-07  5:19           ` Paul Moore
  0 siblings, 1 reply; 15+ messages in thread
From: James Morris @ 2019-06-07  0:45 UTC (permalink / raw)
  To: Paul Moore
  Cc: Casey Schaufler, Janne Karhunen, Stephen Smalley, Mimi Zohar,
	linux-integrity, linux-security-module

On Wed, 5 Jun 2019, Paul Moore wrote:

> On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> >
> > One hook with an added "bool blocking" argument, if
> > that's the only difference?
> 
> I think there is value in keeping a similar convention to the notifier
> code on which this is based, see include/linux/notifier.h.
> 

Although this doesn't seem to be what other users in the kernel are doing. 
Probably the less code churn the better in this case.


-- 
James Morris
<jmorris@namei.org>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-07  0:45         ` James Morris
@ 2019-06-07  5:19           ` Paul Moore
  2019-06-07 21:48             ` James Morris
  0 siblings, 1 reply; 15+ messages in thread
From: Paul Moore @ 2019-06-07  5:19 UTC (permalink / raw)
  To: James Morris
  Cc: Casey Schaufler, Janne Karhunen, Stephen Smalley, Mimi Zohar,
	linux-integrity, linux-security-module

On Thu, Jun 6, 2019 at 8:45 PM James Morris <jmorris@namei.org> wrote:
> On Wed, 5 Jun 2019, Paul Moore wrote:
> > On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> > >
> > > One hook with an added "bool blocking" argument, if
> > > that's the only difference?
> >
> > I think there is value in keeping a similar convention to the notifier
> > code on which this is based, see include/linux/notifier.h.
>
> Although this doesn't seem to be what other users in the kernel are doing.

How many of them potentially have the need for both blocking and
non-blocking notifiers?  I didn't go through the entire list of
callers, but it seems all that I looked at used only one type.  The
simple fact that we started with one type of notifier for the LSM, and
we are now switching to the other (and getting lucky that it is safe
to do so for the existing callers) seems to lend some weight to the
argument we may need both and adding "block"/"blocking"/etc. to the
name has value.

-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-07  5:19           ` Paul Moore
@ 2019-06-07 21:48             ` James Morris
  2019-06-09 17:06               ` Janne Karhunen
  0 siblings, 1 reply; 15+ messages in thread
From: James Morris @ 2019-06-07 21:48 UTC (permalink / raw)
  To: Paul Moore
  Cc: Casey Schaufler, Janne Karhunen, Stephen Smalley, Mimi Zohar,
	linux-integrity, linux-security-module

On Fri, 7 Jun 2019, Paul Moore wrote:

> On Thu, Jun 6, 2019 at 8:45 PM James Morris <jmorris@namei.org> wrote:
> > On Wed, 5 Jun 2019, Paul Moore wrote:
> > > On Wed, Jun 5, 2019 at 1:05 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> > > > On 6/5/2019 9:51 AM, Janne Karhunen wrote:
> > > >
> > > > One hook with an added "bool blocking" argument, if
> > > > that's the only difference?
> > >
> > > I think there is value in keeping a similar convention to the notifier
> > > code on which this is based, see include/linux/notifier.h.
> >
> > Although this doesn't seem to be what other users in the kernel are doing.
> 
> How many of them potentially have the need for both blocking and
> non-blocking notifiers?  I didn't go through the entire list of
> callers, but it seems all that I looked at used only one type.  The
> simple fact that we started with one type of notifier for the LSM, and
> we are now switching to the other (and getting lucky that it is safe
> to do so for the existing callers) seems to lend some weight to the
> argument we may need both and adding "block"/"blocking"/etc. to the
> name has value.

Fair enough.


-- 
James Morris
<jmorris@namei.org>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-06-07 21:48             ` James Morris
@ 2019-06-09 17:06               ` Janne Karhunen
  0 siblings, 0 replies; 15+ messages in thread
From: Janne Karhunen @ 2019-06-09 17:06 UTC (permalink / raw)
  To: James Morris
  Cc: Paul Moore, Casey Schaufler, Stephen Smalley, Mimi Zohar,
	linux-integrity, linux-security-module

On Sat, Jun 8, 2019 at 12:48 AM James Morris <jmorris@namei.org> wrote:

> > simple fact that we started with one type of notifier for the LSM, and
> > we are now switching to the other (and getting lucky that it is safe
> > to do so for the existing callers) seems to lend some weight to the
> > argument we may need both and adding "block"/"blocking"/etc. to the
> > name has value.
>
> Fair enough.

Ok, I take this to mean we have an agreement to go with this variant.
I will post the fixes to the Mimi's findings on top of this one
tomorrow.


--
Janne

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/2] LSM: switch to blocking policy update notifiers
  2019-05-31 14:02 Janne Karhunen
@ 2019-06-03 15:57 ` Paul Moore
  0 siblings, 0 replies; 15+ messages in thread
From: Paul Moore @ 2019-06-03 15:57 UTC (permalink / raw)
  To: Janne Karhunen
  Cc: Stephen Smalley, zohar, linux-integrity, linux-security-module

On Fri, May 31, 2019 at 10:03 AM Janne Karhunen
<janne.karhunen@gmail.com> wrote:
> 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.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
> ---
>  security/security.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 23cbb1a295a3..c5e69ce81521 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -39,7 +39,7 @@
>  #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
>
>  struct security_hook_heads security_hook_heads __lsm_ro_after_init;
> -static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
> +static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);
>
>  static struct kmem_cache *lsm_file_cache;
>  static struct kmem_cache *lsm_inode_cache;
> @@ -432,19 +432,19 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
>
>  int call_lsm_notifier(enum lsm_event event, void *data)

As I mentioned in the other thread, I would like to see "blocking", or
similar, added to the lsm_notifier functions with this change.  It
makes it easier if/when we need to add both atomic and blocking
variants, as well as making it much more clear which version is being
used (helpful even now with just one variant).

For example: call_lsm_notifier() -> call_lsm_blocking_notifier(),
register_lsm_notifier() -> register_lsm_blocking_notifier().

>  {
> -       return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
> +       return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
>  }
>  EXPORT_SYMBOL(call_lsm_notifier);
>
>  int register_lsm_notifier(struct notifier_block *nb)
>  {
> -       return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
> +       return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
>  }
>  EXPORT_SYMBOL(register_lsm_notifier);
>
>  int unregister_lsm_notifier(struct notifier_block *nb)
>  {
> -       return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
> +       return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
>  }
>  EXPORT_SYMBOL(unregister_lsm_notifier);
>
> --
> 2.17.1
>


-- 
paul moore
www.paul-moore.com

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/2] LSM: switch to blocking policy update notifiers
@ 2019-05-31 14:02 Janne Karhunen
  2019-06-03 15:57 ` Paul Moore
  0 siblings, 1 reply; 15+ messages in thread
From: Janne Karhunen @ 2019-05-31 14:02 UTC (permalink / raw)
  To: sds, zohar, paul, linux-integrity, linux-security-module; +Cc: Janne Karhunen

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.

Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
---
 security/security.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/security/security.c b/security/security.c
index 23cbb1a295a3..c5e69ce81521 100644
--- a/security/security.c
+++ b/security/security.c
@@ -39,7 +39,7 @@
 #define LSM_COUNT (__end_lsm_info - __start_lsm_info)
 
 struct security_hook_heads security_hook_heads __lsm_ro_after_init;
-static ATOMIC_NOTIFIER_HEAD(lsm_notifier_chain);
+static BLOCKING_NOTIFIER_HEAD(lsm_notifier_chain);
 
 static struct kmem_cache *lsm_file_cache;
 static struct kmem_cache *lsm_inode_cache;
@@ -432,19 +432,19 @@ void __init security_add_hooks(struct security_hook_list *hooks, int count,
 
 int call_lsm_notifier(enum lsm_event event, void *data)
 {
-	return atomic_notifier_call_chain(&lsm_notifier_chain, event, data);
+	return blocking_notifier_call_chain(&lsm_notifier_chain, event, data);
 }
 EXPORT_SYMBOL(call_lsm_notifier);
 
 int register_lsm_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_register(&lsm_notifier_chain, nb);
+	return blocking_notifier_chain_register(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(register_lsm_notifier);
 
 int unregister_lsm_notifier(struct notifier_block *nb)
 {
-	return atomic_notifier_chain_unregister(&lsm_notifier_chain, nb);
+	return blocking_notifier_chain_unregister(&lsm_notifier_chain, nb);
 }
 EXPORT_SYMBOL(unregister_lsm_notifier);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2019-06-09 17:07 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-05  8:36 [PATCH 1/2] LSM: switch to blocking policy update notifiers Janne Karhunen
2019-06-05  8:36 ` [PATCH 2/2] ima: use the lsm policy update notifier Janne Karhunen
2019-06-06 21:59   ` Mimi Zohar
2019-06-06 22:28     ` Mimi Zohar
2019-06-05 15:23 ` [PATCH 1/2] LSM: switch to blocking policy update notifiers Casey Schaufler
2019-06-05 16:51   ` Janne Karhunen
2019-06-05 17:05     ` Casey Schaufler
2019-06-05 19:14       ` Paul Moore
2019-06-07  0:45         ` James Morris
2019-06-07  5:19           ` Paul Moore
2019-06-07 21:48             ` James Morris
2019-06-09 17:06               ` Janne Karhunen
2019-06-05 19:15 ` Paul Moore
  -- strict thread matches above, loose matches on Subject: below --
2019-05-31 14:02 Janne Karhunen
2019-06-03 15:57 ` Paul Moore

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.