diff mbox series

[v12,08/26] ima: Move IMA securityfs files into ima_namespace or onto stack

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

Commit Message

Stefan Berger April 20, 2022, 2:06 p.m. UTC
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
on systems that don't allow reading or extending the IMA custom policy.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Acked-by: Christian Brauner <brauner@kernel.org>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

---

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

Comments

Serge E. Hallyn May 21, 2022, 3:24 a.m. UTC | #1
On Wed, Apr 20, 2022 at 10:06:15AM -0400, Stefan Berger wrote:
> 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
> on systems that don't allow reading or extending the IMA custom policy.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Acked-by: Christian Brauner <brauner@kernel.org>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

> 
> ---
> 
> v9:
>  - Revert renaming of ima_policy to policy_dentry
> ---
>  security/integrity/ima/ima.h    |  2 ++
>  security/integrity/ima/ima_fs.c | 37 ++++++++++++++++++---------------
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index a144edfdb9a1..b35c8504ef87 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 4cf786f0bba8..89d3113ceda1 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,14 @@ 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;
>  	int ret;
>  
>  	ima_dir = securityfs_create_dir("ima", integrity_dir);
> @@ -504,17 +502,17 @@ int __init ima_fs_init(void)
>  		goto out;
>  	}
>  
> -	ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
> -					    ima_dir, NULL,
> -					    &ima_measure_policy_ops);
> -	if (IS_ERR(ima_policy)) {
> -		ret = PTR_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)) {
> +		ret = PTR_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);
> @@ -524,3 +522,8 @@ int __init ima_fs_init(void)
>  
>  	return ret;
>  }
> +
> +int __init ima_fs_init(void)
> +{
> +	return ima_fs_ns_init(&init_ima_ns);
> +}
> -- 
> 2.34.1
diff mbox series

Patch

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a144edfdb9a1..b35c8504ef87 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 4cf786f0bba8..89d3113ceda1 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,14 @@  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;
 	int ret;
 
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
@@ -504,17 +502,17 @@  int __init ima_fs_init(void)
 		goto out;
 	}
 
-	ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
-					    ima_dir, NULL,
-					    &ima_measure_policy_ops);
-	if (IS_ERR(ima_policy)) {
-		ret = PTR_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)) {
+		ret = PTR_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);
@@ -524,3 +522,8 @@  int __init ima_fs_init(void)
 
 	return ret;
 }
+
+int __init ima_fs_init(void)
+{
+	return ima_fs_ns_init(&init_ima_ns);
+}