diff mbox series

[v10,11/27] ima: Move ima_lsm_policy_notifier into ima_namespace

Message ID 20220201203735.164593-12-stefanb@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series ima: Namespace IMA with audit support in IMA-ns | expand

Commit Message

Stefan Berger Feb. 1, 2022, 8:37 p.m. UTC 
Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
namespace can now register its own LSM policy change notifier callback.
The policy change notifier for the init_ima_ns still remains in init_ima()
and therefore handle the registration of the callback for all other
namespaces in init_ima_namespace().

Suppress the kernel warning 'rule for LSM <label> is undefined` for all
other IMA namespaces than the init_ima_ns.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

---
v10:
 - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
---
 security/integrity/ima/ima.h             |  2 ++
 security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
 security/integrity/ima/ima_main.c        |  6 +-----
 security/integrity/ima/ima_policy.c      | 16 ++++++++++------
 4 files changed, 27 insertions(+), 11 deletions(-)

Comments

Mimi Zohar Feb. 17, 2022, 8:30 p.m. UTC | #1 
On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:

The builtin IMA policy rules are broad and may be constrained by
loading a custom policy, which could be defined in terms of LSM labels.
When an LSM policy is loaded, existing LSM labels might be affected or
even removed.  In either case, IMA policy rules based on LSM
labels, need to reflect these changes.  If an LSM label is removed,
instead of deleting the IMA policy rule based on the LSM label, the IMA
policy rule is made inactive.

> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
> namespace can now register its own LSM policy change notifier callback.
> The policy change notifier for the init_ima_ns still remains in init_ima()
> and therefore handle the registration of the callback for all other
> namespaces in init_ima_namespace().
> 
> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
> other IMA namespaces than the init_ima_ns.

Instead of ignoring the warnings totally, perhaps use either the
"ratelimited" or "once" function options for non init_ima_ns.  It would
be nice if these functions could be namespace aware, so that each
affected IMA namespace would contain at least one warning.

> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> 
> ---
> v10:
>  - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
> ---
>  security/integrity/ima/ima.h             |  2 ++
>  security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
>  security/integrity/ima/ima_main.c        |  6 +-----
>  security/integrity/ima/ima_policy.c      | 16 ++++++++++------
>  4 files changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 94c6e3a4d666..fb6bd054d899 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -144,6 +144,8 @@ struct ima_namespace {
>  	int valid_policy;
>  
>  	struct dentry *ima_policy;
> +
> +	struct notifier_block ima_lsm_policy_notifier;
>  } __randomize_layout;
>  extern struct ima_namespace init_ima_ns;
>  
> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
> index 425eed1c6838..1cba545ae477 100644
> --- a/security/integrity/ima/ima_init_ima_ns.c
> +++ b/security/integrity/ima/ima_init_ima_ns.c
> @@ -10,6 +10,8 @@
>  
>  static int ima_init_namespace(struct ima_namespace *ns)
>  {
> +	int rc;
> +

There's no right or wrong, but the newer IMA code uses 'ret'.

>  	INIT_LIST_HEAD(&ns->ima_default_rules);
>  	INIT_LIST_HEAD(&ns->ima_policy_rules);
>  	INIT_LIST_HEAD(&ns->ima_temp_rules);
Stefan Berger Feb. 17, 2022, 8:59 p.m. UTC | #2 
On 2/17/22 15:30, Mimi Zohar wrote:
> On Tue, 2022-02-01 at 15:37 -0500, Stefan Berger wrote:
>
> The builtin IMA policy rules are broad and may be constrained by
> loading a custom policy, which could be defined in terms of LSM labels.
> When an LSM policy is loaded, existing LSM labels might be affected or
> even removed.  In either case, IMA policy rules based on LSM
> labels, need to reflect these changes.  If an LSM label is removed,
> instead of deleting the IMA policy rule based on the LSM label, the IMA
> policy rule is made inactive.

Ok, so I take this paragraph for the patch description.


>
>> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
>> namespace can now register its own LSM policy change notifier callback.
>> The policy change notifier for the init_ima_ns still remains in init_ima()
>> and therefore handle the registration of the callback for all other
>> namespaces in init_ima_namespace().
>>
>> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
>> other IMA namespaces than the init_ima_ns.
> Instead of ignoring the warnings totally, perhaps use either the
> "ratelimited" or "once" function options for non init_ima_ns.  It would
> be nice if these functions could be namespace aware, so that each
> affected IMA namespace would contain at least one warning.

The problem is that any user can now repeatedly create user namespaces 
and with that IMA namespaces and cause the kernel log to fill up with 
these messages and also flood the audit log -- I guess one could 
describe it as an unwanted side-effect. I am afraid that for as long as 
the kernel log is not namespaced it's probably best to just turn them 
off for non-init_ima_ns.


>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
Thanks.
>
>> ---
>> v10:
>>   - Only call pr_warn('rule for LSM <label> is undefined`) for init_ima_ns
>> ---
>>   security/integrity/ima/ima.h             |  2 ++
>>   security/integrity/ima/ima_init_ima_ns.c | 14 ++++++++++++++
>>   security/integrity/ima/ima_main.c        |  6 +-----
>>   security/integrity/ima/ima_policy.c      | 16 ++++++++++------
>>   4 files changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index 94c6e3a4d666..fb6bd054d899 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -144,6 +144,8 @@ struct ima_namespace {
>>   	int valid_policy;
>>   
>>   	struct dentry *ima_policy;
>> +
>> +	struct notifier_block ima_lsm_policy_notifier;
>>   } __randomize_layout;
>>   extern struct ima_namespace init_ima_ns;
>>   
>> diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
>> index 425eed1c6838..1cba545ae477 100644
>> --- a/security/integrity/ima/ima_init_ima_ns.c
>> +++ b/security/integrity/ima/ima_init_ima_ns.c
>> @@ -10,6 +10,8 @@
>>   
>>   static int ima_init_namespace(struct ima_namespace *ns)
>>   {
>> +	int rc;
>> +
> There's no right or wrong, but the newer IMA code uses 'ret'.


I don't mind to change it. I will take your Reviewed-by, though :-)


>
>>   	INIT_LIST_HEAD(&ns->ima_default_rules);
>>   	INIT_LIST_HEAD(&ns->ima_policy_rules);
>>   	INIT_LIST_HEAD(&ns->ima_temp_rules);
Mimi Zohar Feb. 17, 2022, 9:24 p.m. UTC | #3 
On Thu, 2022-02-17 at 15:59 -0500, Stefan Berger wrote:
> >
> >> Move the ima_lsm_policy_notifier into the ima_namespace. Each IMA
> >> namespace can now register its own LSM policy change notifier callback.
> >> The policy change notifier for the init_ima_ns still remains in init_ima()
> >> and therefore handle the registration of the callback for all other
> >> namespaces in init_ima_namespace().
> >>
> >> Suppress the kernel warning 'rule for LSM <label> is undefined` for all
> >> other IMA namespaces than the init_ima_ns.
> > Instead of ignoring the warnings totally, perhaps use either the
> > "ratelimited" or "once" function options for non init_ima_ns.  It would
> > be nice if these functions could be namespace aware, so that each
> > affected IMA namespace would contain at least one warning.
> 
> The problem is that any user can now repeatedly create user namespaces 
> and with that IMA namespaces and cause the kernel log to fill up with 
> these messages and also flood the audit log -- I guess one could 
> describe it as an unwanted side-effect. I am afraid that for as long as 
> the kernel log is not namespaced it's probably best to just turn them 
> off for non-init_ima_ns.

There are functions - pr_warn_once() or pr_warn_ratelimited() - that
limit the number of kernel messages.  In addition to the number of
potential kernel messages, there's also the issue of being able to
differentiate between init_ima_ns and other IMA namespaces.  I think
that is more of a concern than rate limiting.
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 94c6e3a4d666..fb6bd054d899 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -144,6 +144,8 @@  struct ima_namespace {
 	int valid_policy;
 
 	struct dentry *ima_policy;
+
+	struct notifier_block ima_lsm_policy_notifier;
 } __randomize_layout;
 extern struct ima_namespace init_ima_ns;
 
diff --git a/security/integrity/ima/ima_init_ima_ns.c b/security/integrity/ima/ima_init_ima_ns.c
index 425eed1c6838..1cba545ae477 100644
--- a/security/integrity/ima/ima_init_ima_ns.c
+++ b/security/integrity/ima/ima_init_ima_ns.c
@@ -10,6 +10,8 @@ 
 
 static int ima_init_namespace(struct ima_namespace *ns)
 {
+	int rc;
+
 	INIT_LIST_HEAD(&ns->ima_default_rules);
 	INIT_LIST_HEAD(&ns->ima_policy_rules);
 	INIT_LIST_HEAD(&ns->ima_temp_rules);
@@ -30,6 +32,15 @@  static int ima_init_namespace(struct ima_namespace *ns)
 	ns->valid_policy = 1;
 	ns->ima_fs_flags = 0;
 
+	if (ns != &init_ima_ns) {
+		ns->ima_lsm_policy_notifier.notifier_call =
+						ima_lsm_policy_change;
+		rc = register_blocking_lsm_notifier
+						(&ns->ima_lsm_policy_notifier);
+		if (rc)
+			return rc;
+	}
+
 	return 0;
 }
 
@@ -39,5 +50,8 @@  int __init ima_ns_init(void)
 }
 
 struct ima_namespace init_ima_ns = {
+	.ima_lsm_policy_notifier = {
+		.notifier_call = ima_lsm_policy_change,
+	},
 };
 EXPORT_SYMBOL(init_ima_ns);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 2cd5cc90ab79..ae0e9b14554a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -38,10 +38,6 @@  int ima_appraise;
 int __ro_after_init 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();
@@ -1072,7 +1068,7 @@  static int __init init_ima(void)
 	if (error)
 		return error;
 
-	error = register_blocking_lsm_notifier(&ima_lsm_policy_notifier);
+	error = register_blocking_lsm_notifier(&ns->ima_lsm_policy_notifier);
 	if (error)
 		pr_warn("Couldn't register LSM notifier, error %d\n", error);
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 05b2bc06ab0c..148ff5a98a8e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -369,7 +369,8 @@  static void ima_free_rule(struct ima_rule_entry *entry)
 	kfree(entry);
 }
 
-static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
+static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_namespace *ns,
+						struct ima_rule_entry *entry)
 {
 	struct ima_rule_entry *nentry;
 	int i;
@@ -400,18 +401,19 @@  static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
 		ima_filter_rule_init(nentry->lsm[i].type, Audit_equal,
 				     nentry->lsm[i].args_p,
 				     &nentry->lsm[i].rule);
-		if (!nentry->lsm[i].rule)
+		if (!nentry->lsm[i].rule && ns == &init_ima_ns)
 			pr_warn("rule for LSM \'%s\' is undefined\n",
 				nentry->lsm[i].args_p);
 	}
 	return nentry;
 }
 
-static int ima_lsm_update_rule(struct ima_rule_entry *entry)
+static int ima_lsm_update_rule(struct ima_namespace *ns,
+			       struct ima_rule_entry *entry)
 {
 	struct ima_rule_entry *nentry;
 
-	nentry = ima_lsm_copy_rule(entry);
+	nentry = ima_lsm_copy_rule(ns, entry);
 	if (!nentry)
 		return -ENOMEM;
 
@@ -454,7 +456,7 @@  static void ima_lsm_update_rules(struct ima_namespace *ns)
 		if (!ima_rule_contains_lsm_cond(entry))
 			continue;
 
-		result = ima_lsm_update_rule(entry);
+		result = ima_lsm_update_rule(ns, entry);
 		if (result) {
 			pr_err("lsm rule update error %d\n", result);
 			return;
@@ -465,12 +467,14 @@  static void ima_lsm_update_rules(struct ima_namespace *ns)
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data)
 {
-	struct ima_namespace *ns = &init_ima_ns;
+	struct ima_namespace *ns;
 
 	if (event != LSM_POLICY_CHANGE)
 		return NOTIFY_DONE;
 
+	ns = container_of(nb, struct ima_namespace, ima_lsm_policy_notifier);
 	ima_lsm_update_rules(ns);
+
 	return NOTIFY_OK;
 }