diff mbox series

[v9,10/23] ima: Move IMA securityfs files into ima_namespace or onto stack

Message ID 20220125224645.79319-11-stefanb@linux.vnet.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 Jan. 25, 2022, 10:46 p.m. UTC 
From: Stefan Berger <stefanb@linux.ibm.com>

Only the securityfs IMA policy file is ever removed based on Kconfig
options. For this reason, move the IMA securityfs policy file variable
'ima_policy' into the ima_namespace.

Move the other IMA securityfs files onto the stack since they are not
needed outside the function where they are created in. Also, their cleanup
is automatically handled by the filesystem upon umount of a virtualized
securityfs instance, so they don't need to be explicitly freed.

In the failure cleanup path clean up the ima_policy dentry before
cleaning up the directories.

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

---

v9:
 - Revert renaming of ima_policy to policy_dentry
---
 security/integrity/ima/ima.h    |  2 ++
 security/integrity/ima/ima_fs.c | 36 ++++++++++++++++++---------------
 2 files changed, 22 insertions(+), 16 deletions(-)

Comments

Christian Brauner Jan. 26, 2022, 9:40 a.m. UTC | #1 
On Tue, Jan 25, 2022 at 05:46:32PM -0500, Stefan Berger wrote:
> From: Stefan Berger <stefanb@linux.ibm.com>
> 
> Only the securityfs IMA policy file is ever removed based on Kconfig
> options. For this reason, move the IMA securityfs policy file variable
> 'ima_policy' into the ima_namespace.
> 
> Move the other IMA securityfs files onto the stack since they are not
> needed outside the function where they are created in. Also, their cleanup
> is automatically handled by the filesystem upon umount of a virtualized
> securityfs instance, so they don't need to be explicitly freed.

I'd reverse the explantion in the commit and mention the securityfs
change that makes this move possible which is patch 3 in this version of
the series ("securityfs: rework dentry creation"); something like:

	Earlier we simplified how dentry creation and deletion is manged in
	securityfs. This allows us to move IMA securityfs files from global
	variables directly into ima_fs_ns_init() itself. We can now rely on
	those dentries to be cleaned up when the securityfs instance is cleaned
	when the last reference to it is dropped.
	
	Things are slightly different for the initial ima namespace. In contrast
	to non-initial ima namespaces it has pinning logic binding the lifetime
	of the securityfs superblock to created dentries. We need to keep this
	behavior to not regress userspace. Since ima never removes most of the
	securityfs files the initial securityfs instance stays pinned. This also
	means even for the initial ima namespace we don't need to keep
	references to these dentries anywhere.
	
	The ima_policy file is the exception since ima can end up removing it if
	a non-default policy is written at some point.

Last sentence should be checked for sensibility by ima folks.

> 
> In the failure cleanup path clean up the ima_policy dentry before
> cleaning up the directories.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> ---

Moving into imans looks good,
Acked-by: Christian Brauner <brauner@kernel.org>
Stefan Berger Jan. 27, 2022, 5:02 p.m. UTC | #2 
On 1/26/22 04:40, Christian Brauner wrote:
> On Tue, Jan 25, 2022 at 05:46:32PM -0500, Stefan Berger wrote:
>> From: Stefan Berger <stefanb@linux.ibm.com>
>>
>> Only the securityfs IMA policy file is ever removed based on Kconfig
>> options. For this reason, move the IMA securityfs policy file variable
>> 'ima_policy' into the ima_namespace.
>>
>> Move the other IMA securityfs files onto the stack since they are not
>> needed outside the function where they are created in. Also, their cleanup
>> is automatically handled by the filesystem upon umount of a virtualized
>> securityfs instance, so they don't need to be explicitly freed.
> I'd reverse the explantion in the commit and mention the securityfs
> change that makes this move possible which is patch 3 in this version of
> the series ("securityfs: rework dentry creation"); something like:
>
> 	Earlier we simplified how dentry creation and deletion is manged in
> 	securityfs. This allows us to move IMA securityfs files from global
> 	variables directly into ima_fs_ns_init() itself. We can now rely on
> 	those dentries to be cleaned up when the securityfs instance is cleaned
> 	when the last reference to it is dropped.
> 	
> 	Things are slightly different for the initial ima namespace. In contrast
> 	to non-initial ima namespaces it has pinning logic binding the lifetime
> 	of the securityfs superblock to created dentries. We need to keep this
> 	behavior to not regress userspace. Since ima never removes most of the
> 	securityfs files the initial securityfs instance stays pinned. This also
> 	means even for the initial ima namespace we don't need to keep
> 	references to these dentries anywhere.
> 	
> 	The ima_policy file is the exception since ima can end up removing it if
> 	a non-default policy is written at some point.
>
> Last sentence should be checked for sensibility by ima folks.

I remove the if clause and took the other text as-is...

Thanks.

    Stefan
Mimi Zohar Jan. 28, 2022, 3:13 a.m. UTC | #3 
On Thu, 2022-01-27 at 12:02 -0500, Stefan Berger wrote:
> On 1/26/22 04:40, Christian Brauner wrote:
> > On Tue, Jan 25, 2022 at 05:46:32PM -0500, Stefan Berger wrote:
> >> From: Stefan Berger <stefanb@linux.ibm.com>
> >>
> >> Only the securityfs IMA policy file is ever removed based on Kconfig
> >> options. For this reason, move the IMA securityfs policy file variable
> >> 'ima_policy' into the ima_namespace.
> >>
> >> Move the other IMA securityfs files onto the stack since they are not
> >> needed outside the function where they are created in. Also, their cleanup
> >> is automatically handled by the filesystem upon umount of a virtualized
> >> securityfs instance, so they don't need to be explicitly freed.
> > I'd reverse the explantion in the commit and mention the securityfs
> > change that makes this move possible which is patch 3 in this version of
> > the series ("securityfs: rework dentry creation"); something like:
> >
> > 	Earlier we simplified how dentry creation and deletion is manged in
> > 	securityfs. This allows us to move IMA securityfs files from global
> > 	variables directly into ima_fs_ns_init() itself. We can now rely on
> > 	those dentries to be cleaned up when the securityfs instance is cleaned
> > 	when the last reference to it is dropped.
> > 	
> > 	Things are slightly different for the initial ima namespace. In contrast
> > 	to non-initial ima namespaces it has pinning logic binding the lifetime
> > 	of the securityfs superblock to created dentries. We need to keep this
> > 	behavior to not regress userspace. Since ima never removes most of the
> > 	securityfs files the initial securityfs instance stays pinned. This also
> > 	means even for the initial ima namespace we don't need to keep
> > 	references to these dentries anywhere.
> > 	
> > 	The ima_policy file is the exception since ima can end up removing it if
> > 	a non-default policy is written at some point.
> >
> > Last sentence should be checked for sensibility by ima folks.
> 
> I remove the if clause and took the other text as-is...

Or replace it with, "on systems that don't allow reading or extending
the IMA custom policy."

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 1092c926daf9..94c6e3a4d666 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -142,6 +142,8 @@  struct ima_namespace {
 	struct mutex ima_write_mutex;
 	unsigned long ima_fs_flags;
 	int valid_policy;
+
+	struct dentry *ima_policy;
 } __randomize_layout;
 extern struct ima_namespace init_ima_ns;
 
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 34132cc7de4d..3afb7a74d2cf 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -359,14 +359,6 @@  static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 	return result;
 }
 
-static struct dentry *ima_dir;
-static struct dentry *ima_symlink;
-static struct dentry *binary_runtime_measurements;
-static struct dentry *ascii_runtime_measurements;
-static struct dentry *runtime_measurements_count;
-static struct dentry *violations;
-static struct dentry *ima_policy;
-
 enum ima_fs_flags {
 	IMA_FS_BUSY,
 };
@@ -436,8 +428,8 @@  static int ima_release_policy(struct inode *inode, struct file *file)
 
 	ima_update_policy(ns);
 #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
-	securityfs_remove(ima_policy);
-	ima_policy = NULL;
+	securityfs_remove(ns->ima_policy);
+	ns->ima_policy = NULL;
 #elif defined(CONFIG_IMA_WRITE_POLICY)
 	clear_bit(IMA_FS_BUSY, &ns->ima_fs_flags);
 #elif defined(CONFIG_IMA_READ_POLICY)
@@ -454,8 +446,15 @@  static const struct file_operations ima_measure_policy_ops = {
 	.llseek = generic_file_llseek,
 };
 
-int __init ima_fs_init(void)
+static int __init ima_fs_ns_init(struct ima_namespace *ns)
 {
+	struct dentry *ima_dir;
+	struct dentry *ima_symlink = NULL;
+	struct dentry *binary_runtime_measurements = NULL;
+	struct dentry *ascii_runtime_measurements = NULL;
+	struct dentry *runtime_measurements_count = NULL;
+	struct dentry *violations = NULL;
+
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
 	if (IS_ERR(ima_dir))
 		return -1;
@@ -492,15 +491,15 @@  int __init ima_fs_init(void)
 	if (IS_ERR(violations))
 		goto out;
 
-	ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
-					    ima_dir, NULL,
-					    &ima_measure_policy_ops);
-	if (IS_ERR(ima_policy))
+	ns->ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
+						ima_dir, NULL,
+						&ima_measure_policy_ops);
+	if (IS_ERR(ns->ima_policy))
 		goto out;
 
 	return 0;
 out:
-	securityfs_remove(ima_policy);
+	securityfs_remove(ns->ima_policy);
 	securityfs_remove(violations);
 	securityfs_remove(runtime_measurements_count);
 	securityfs_remove(ascii_runtime_measurements);
@@ -509,3 +508,8 @@  int __init ima_fs_init(void)
 	securityfs_remove(ima_dir);
 	return -1;
 }
+
+int __init ima_fs_init(void)
+{
+	return ima_fs_ns_init(&init_ima_ns);
+}