[PATCHES][CFR][CFT] securityfs cleanups and fixes

[PATCHES][CFR][CFT] securityfs cleanups and fixes

[Al Viro]

	Resurrected and somewhat fixed series of securityfs
cleanups and fixes:

* one extra reference is enough to pin a dentry down; no need
for two.  Switch to regular scheme, similar to shmem, debugfs,
etc. - that fixes securityfs_recursive_remove() dentry leak,
among other things.

* we need to have the filesystem pinned to prevent the contents
disappearing; what we do not need is pinning it for each file.
Doing that only for files and directories in the root is enough.

* the previous two changes allow to get rid of the racy kludges
in efi_secret_unlink(), where we can use simple_unlink() instead
of securityfs_remove().  Which does not require unlocking and
relocking the parent, with all deadlocks that invites.

* Make securityfs_remove() take the entire subtree out, turning
securityfs_recursive_remove() into its alias.  Makes a lot more
sense for callers and fixes a mount leak, while we are at it.

* Making securityfs_remove() remove the entire subtree allows for
much simpler life in most of the users - efi_secret, ima_fs,
evm, ipe, tmp get cleaner.  I hadn't touched apparmor use of
securityfs, but I suspect that it would be useful there as well.

Branch (6.16-rc1-based) lives in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.securityfs
Individual patches in followups.

Help with testing and review would be very welcome.

Shortlog:
      securityfs: don't pin dentries twice, once is enough...
      securityfs: pin filesystem only for objects directly in root
      fix locking in efi_secret_unlink()
      make securityfs_remove() remove the entire subtree
      efi_secret: clean securityfs use up
      ima_fs: don't bother with removal of files in directory we'll be removing
      ima_fs: get rid of lookup-by-dentry stuff
      evm_secfs: clear securityfs interactions
      ipe: don't bother with removal of files in directory we'll be removing
      tpm: don't bother with removal of files in directory we'll be removing

Diffstat:

 drivers/char/tpm/eventlog/common.c        |  46 +++-------
 drivers/virt/coco/efi_secret/efi_secret.c |  47 ++--------
 include/linux/security.h                  |   3 +-
 include/linux/tpm.h                       |   2 +-
 security/inode.c                          |  62 +++++---------
 security/integrity/evm/evm_secfs.c        |  15 ++--
 security/integrity/ima/ima_fs.c           | 137 +++++++-----------------------
 security/ipe/fs.c                         |  32 +++----
 security/ipe/policy_fs.c                  |   4 +-
 9 files changed, 97 insertions(+), 251 deletions(-)

[PATCH 01/10] securityfs: don't pin dentries twice, once is enough...

[Al Viro]

incidentally, securityfs_recursive_remove() is broken without that -
it leaks dentries, since simple_recursive_removal() does not expect
anything of that sort.  It could be worked around by dput() in
remove_one() callback, but it's easier to just drop that double-get
stuff.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/inode.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index 3913501621fa..93460eab8216 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -159,7 +159,6 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 		inode->i_fop = fops;
 	}
 	d_instantiate(dentry, inode);
-	dget(dentry);
 	inode_unlock(dir);
 	return dentry;
 
@@ -306,7 +305,6 @@ void securityfs_remove(struct dentry *dentry)
 			simple_rmdir(dir, dentry);
 		else
 			simple_unlink(dir, dentry);
-		dput(dentry);
 	}
 	inode_unlock(dir);
 	simple_release_fs(&mount, &mount_count);
-- 
2.39.5

[PATCH 02/10] securityfs: pin filesystem only for objects directly in root

[Al Viro]

Nothing on securityfs ever changes parents, so we don't need
to pin the internal mount if it's already pinned for parent.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/inode.c | 21 +++++++++++++--------
 1 file changed, 13 insertions(+), 8 deletions(-)

diff --git a/security/inode.c b/security/inode.c
index 93460eab8216..1ecb8859c272 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -112,18 +112,20 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 	struct dentry *dentry;
 	struct inode *dir, *inode;
 	int error;
+	bool pinned = false;
 
 	if (!(mode & S_IFMT))
 		mode = (mode & S_IALLUGO) | S_IFREG;
 
 	pr_debug("securityfs: creating file '%s'\n",name);
 
-	error = simple_pin_fs(&fs_type, &mount, &mount_count);
-	if (error)
-		return ERR_PTR(error);
-
-	if (!parent)
+	if (!parent) {
+		error = simple_pin_fs(&fs_type, &mount, &mount_count);
+		if (error)
+			return ERR_PTR(error);
+		pinned = true;
 		parent = mount->mnt_root;
+	}
 
 	dir = d_inode(parent);
 
@@ -167,7 +169,8 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
 	dentry = ERR_PTR(error);
 out:
 	inode_unlock(dir);
-	simple_release_fs(&mount, &mount_count);
+	if (pinned)
+		simple_release_fs(&mount, &mount_count);
 	return dentry;
 }
 
@@ -307,13 +310,15 @@ void securityfs_remove(struct dentry *dentry)
 			simple_unlink(dir, dentry);
 	}
 	inode_unlock(dir);
-	simple_release_fs(&mount, &mount_count);
+	if (dir == dir->i_sb->s_root->d_inode)
+		simple_release_fs(&mount, &mount_count);
 }
 EXPORT_SYMBOL_GPL(securityfs_remove);
 
 static void remove_one(struct dentry *victim)
 {
-	simple_release_fs(&mount, &mount_count);
+	if (victim->d_parent == victim->d_sb->s_root)
+		simple_release_fs(&mount, &mount_count);
 }
 
 /**
-- 
2.39.5

[PATCH 03/10] fix locking in efi_secret_unlink()

[Al Viro]

We used to need securityfs_remove() to undo simple_pin_fs() done when
the file had been created and to drop the second extra reference
taken at the same time.  Now that neither is needed (or done by
securityfs_remove()), we can simply call simple_unlink() and be done
with that - the broken games with locking had been there only for the
sake of securityfs_remove().

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/virt/coco/efi_secret/efi_secret.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
index 1864f9f80617..f2da4819ec3b 100644
--- a/drivers/virt/coco/efi_secret/efi_secret.c
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -136,15 +136,7 @@ static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 		if (s->fs_files[i] == dentry)
 			s->fs_files[i] = NULL;
 
-	/*
-	 * securityfs_remove tries to lock the directory's inode, but we reach
-	 * the unlink callback when it's already locked
-	 */
-	inode_unlock(dir);
-	securityfs_remove(dentry);
-	inode_lock(dir);
-
-	return 0;
+	return simple_unlink(inode, dentry);
 }
 
 static const struct inode_operations efi_secret_dir_inode_operations = {
-- 
2.39.5

[PATCH 04/10] make securityfs_remove() remove the entire subtree

[Al Viro]

... and fix the mount leak when anything's mounted there.
securityfs_recursive_remove becomes an alias for securityfs_remove -
we'll probably need to remove it in a cycle or two.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 include/linux/security.h |  3 ++-
 security/inode.c         | 47 +++++++++-------------------------------
 2 files changed, 12 insertions(+), 38 deletions(-)

diff --git a/include/linux/security.h b/include/linux/security.h
index dba349629229..386463b5e848 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -2211,7 +2211,6 @@ struct dentry *securityfs_create_symlink(const char *name,
 					 const char *target,
 					 const struct inode_operations *iops);
 extern void securityfs_remove(struct dentry *dentry);
-extern void securityfs_recursive_remove(struct dentry *dentry);
 
 #else /* CONFIG_SECURITYFS */
 
@@ -2243,6 +2242,8 @@ static inline void securityfs_remove(struct dentry *dentry)
 
 #endif
 
+#define securityfs_recursive_remove securityfs_remove
+
 #ifdef CONFIG_BPF_SYSCALL
 union bpf_attr;
 struct bpf_map;
diff --git a/security/inode.c b/security/inode.c
index 1ecb8859c272..43382ef8896e 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -281,6 +281,12 @@ struct dentry *securityfs_create_symlink(const char *name,
 }
 EXPORT_SYMBOL_GPL(securityfs_create_symlink);
 
+static void remove_one(struct dentry *victim)
+{
+	if (victim->d_parent == victim->d_sb->s_root)
+		simple_release_fs(&mount, &mount_count);
+}
+
 /**
  * securityfs_remove - removes a file or directory from the securityfs filesystem
  *
@@ -293,44 +299,11 @@ EXPORT_SYMBOL_GPL(securityfs_create_symlink);
  * This function is required to be called in order for the file to be
  * removed. No automatic cleanup of files will happen when a module is
  * removed; you are responsible here.
- */
-void securityfs_remove(struct dentry *dentry)
-{
-	struct inode *dir;
-
-	if (IS_ERR_OR_NULL(dentry))
-		return;
-
-	dir = d_inode(dentry->d_parent);
-	inode_lock(dir);
-	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(dir, dentry);
-		else
-			simple_unlink(dir, dentry);
-	}
-	inode_unlock(dir);
-	if (dir == dir->i_sb->s_root->d_inode)
-		simple_release_fs(&mount, &mount_count);
-}
-EXPORT_SYMBOL_GPL(securityfs_remove);
-
-static void remove_one(struct dentry *victim)
-{
-	if (victim->d_parent == victim->d_sb->s_root)
-		simple_release_fs(&mount, &mount_count);
-}
-
-/**
- * securityfs_recursive_remove - recursively removes a file or directory
- *
- * @dentry: a pointer to a the dentry of the file or directory to be removed.
  *
- * This function recursively removes a file or directory in securityfs that was
- * previously created with a call to another securityfs function (like
- * securityfs_create_file() or variants thereof.)
+ * AV: when applied to directory it will take all children out; no need to call
+ * it for descendents if ancestor is getting killed.
  */
-void securityfs_recursive_remove(struct dentry *dentry)
+void securityfs_remove(struct dentry *dentry)
 {
 	if (IS_ERR_OR_NULL(dentry))
 		return;
@@ -339,7 +312,7 @@ void securityfs_recursive_remove(struct dentry *dentry)
 	simple_recursive_removal(dentry, remove_one);
 	simple_release_fs(&mount, &mount_count);
 }
-EXPORT_SYMBOL_GPL(securityfs_recursive_remove);
+EXPORT_SYMBOL_GPL(securityfs_remove);
 
 #ifdef CONFIG_SECURITY
 static struct dentry *lsm_dentry;
-- 
2.39.5

[PATCH 05/10] efi_secret: clean securityfs use up

[Al Viro]

securityfs_remove() does take care of entire subtree now; no need
to mess with them individually.

NB: ->i_op replacement in there is still buggy.  One shouldn't
ever modify ->i_op of live accessible inode.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/virt/coco/efi_secret/efi_secret.c | 37 +++++------------------
 1 file changed, 8 insertions(+), 29 deletions(-)

diff --git a/drivers/virt/coco/efi_secret/efi_secret.c b/drivers/virt/coco/efi_secret/efi_secret.c
index f2da4819ec3b..5946c5abeae8 100644
--- a/drivers/virt/coco/efi_secret/efi_secret.c
+++ b/drivers/virt/coco/efi_secret/efi_secret.c
@@ -31,8 +31,6 @@
 
 struct efi_secret {
 	struct dentry *secrets_dir;
-	struct dentry *fs_dir;
-	struct dentry *fs_files[EFI_SECRET_NUM_FILES];
 	void __iomem *secret_data;
 	u64 secret_data_len;
 };
@@ -119,10 +117,8 @@ static void wipe_memory(void *addr, size_t size)
 
 static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 {
-	struct efi_secret *s = efi_secret_get();
 	struct inode *inode = d_inode(dentry);
 	struct secret_entry *e = (struct secret_entry *)inode->i_private;
-	int i;
 
 	if (e) {
 		/* Zero out the secret data */
@@ -132,10 +128,6 @@ static int efi_secret_unlink(struct inode *dir, struct dentry *dentry)
 
 	inode->i_private = NULL;
 
-	for (i = 0; i < EFI_SECRET_NUM_FILES; i++)
-		if (s->fs_files[i] == dentry)
-			s->fs_files[i] = NULL;
-
 	return simple_unlink(inode, dentry);
 }
 
@@ -186,15 +178,6 @@ static int efi_secret_map_area(struct platform_device *dev)
 static void efi_secret_securityfs_teardown(struct platform_device *dev)
 {
 	struct efi_secret *s = efi_secret_get();
-	int i;
-
-	for (i = (EFI_SECRET_NUM_FILES - 1); i >= 0; i--) {
-		securityfs_remove(s->fs_files[i]);
-		s->fs_files[i] = NULL;
-	}
-
-	securityfs_remove(s->fs_dir);
-	s->fs_dir = NULL;
 
 	securityfs_remove(s->secrets_dir);
 	s->secrets_dir = NULL;
@@ -209,7 +192,7 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	unsigned char *ptr;
 	struct secret_header *h;
 	struct secret_entry *e;
-	struct dentry *dent;
+	struct dentry *dent, *dir;
 	char guid_str[EFI_VARIABLE_GUID_LEN + 1];
 
 	ptr = (void __force *)s->secret_data;
@@ -232,8 +215,6 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	}
 
 	s->secrets_dir = NULL;
-	s->fs_dir = NULL;
-	memset(s->fs_files, 0, sizeof(s->fs_files));
 
 	dent = securityfs_create_dir("secrets", NULL);
 	if (IS_ERR(dent)) {
@@ -243,14 +224,13 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 	}
 	s->secrets_dir = dent;
 
-	dent = securityfs_create_dir("coco", s->secrets_dir);
-	if (IS_ERR(dent)) {
+	dir = securityfs_create_dir("coco", s->secrets_dir);
+	if (IS_ERR(dir)) {
 		dev_err(&dev->dev, "Error creating coco securityfs directory entry err=%ld\n",
-			PTR_ERR(dent));
-		return PTR_ERR(dent);
+			PTR_ERR(dir));
+		return PTR_ERR(dir);
 	}
-	d_inode(dent)->i_op = &efi_secret_dir_inode_operations;
-	s->fs_dir = dent;
+	d_inode(dir)->i_op = &efi_secret_dir_inode_operations;
 
 	bytes_left = h->len - sizeof(*h);
 	ptr += sizeof(*h);
@@ -266,15 +246,14 @@ static int efi_secret_securityfs_setup(struct platform_device *dev)
 		if (efi_guidcmp(e->guid, NULL_GUID)) {
 			efi_guid_to_str(&e->guid, guid_str);
 
-			dent = securityfs_create_file(guid_str, 0440, s->fs_dir, (void *)e,
+			dent = securityfs_create_file(guid_str, 0440, dir, (void *)e,
 						      &efi_secret_bin_file_fops);
 			if (IS_ERR(dent)) {
 				dev_err(&dev->dev, "Error creating efi_secret securityfs entry\n");
 				ret = PTR_ERR(dent);
 				goto err_cleanup;
 			}
-
-			s->fs_files[i++] = dent;
+			i++;
 		}
 		ptr += e->len;
 		bytes_left -= e->len;
-- 
2.39.5

[PATCH 06/10] ima_fs: don't bother with removal of files in directory we'll be removing

[Al Viro]

removal of parent takes all children out

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/ima/ima_fs.c | 57 +++++++++++----------------------
 1 file changed, 18 insertions(+), 39 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e4a79a9b2d58..88421e8895c4 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -396,11 +396,6 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 
 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,
@@ -419,14 +414,7 @@ static const struct seq_operations ima_policy_seqops = {
 
 static void __init remove_securityfs_measurement_lists(struct dentry **lists)
 {
-	int i;
-
-	if (lists) {
-		for (i = 0; i < securityfs_measurement_list_count; i++)
-			securityfs_remove(lists[i]);
-
-		kfree(lists);
-	}
+	kfree(lists);
 }
 
 static int __init create_securityfs_measurement_lists(void)
@@ -533,8 +521,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 
 	ima_update_policy();
 #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
-	securityfs_remove(ima_policy);
-	ima_policy = NULL;
+	securityfs_remove(file->f_path.dentry);
 #elif defined(CONFIG_IMA_WRITE_POLICY)
 	clear_bit(IMA_FS_BUSY, &ima_fs_flags);
 #elif defined(CONFIG_IMA_READ_POLICY)
@@ -553,6 +540,7 @@ static const struct file_operations ima_measure_policy_ops = {
 
 int __init ima_fs_init(void)
 {
+	struct dentry *dentry;
 	int ret;
 
 	ascii_securityfs_measurement_lists = NULL;
@@ -573,54 +561,45 @@ int __init ima_fs_init(void)
 	if (ret != 0)
 		goto out;
 
-	binary_runtime_measurements =
-	    securityfs_create_symlink("binary_runtime_measurements", ima_dir,
+	dentry = securityfs_create_symlink("binary_runtime_measurements", ima_dir,
 				      "binary_runtime_measurements_sha1", NULL);
-	if (IS_ERR(binary_runtime_measurements)) {
-		ret = PTR_ERR(binary_runtime_measurements);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	ascii_runtime_measurements =
-	    securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
+	dentry = securityfs_create_symlink("ascii_runtime_measurements", ima_dir,
 				      "ascii_runtime_measurements_sha1", NULL);
-	if (IS_ERR(ascii_runtime_measurements)) {
-		ret = PTR_ERR(ascii_runtime_measurements);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	runtime_measurements_count =
-	    securityfs_create_file("runtime_measurements_count",
+	dentry = securityfs_create_file("runtime_measurements_count",
 				   S_IRUSR | S_IRGRP, ima_dir, NULL,
 				   &ima_measurements_count_ops);
-	if (IS_ERR(runtime_measurements_count)) {
-		ret = PTR_ERR(runtime_measurements_count);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	violations =
-	    securityfs_create_file("violations", S_IRUSR | S_IRGRP,
+	dentry = securityfs_create_file("violations", S_IRUSR | S_IRGRP,
 				   ima_dir, NULL, &ima_htable_violations_ops);
-	if (IS_ERR(violations)) {
-		ret = PTR_ERR(violations);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
-	ima_policy = securityfs_create_file("policy", POLICY_FILE_FLAGS,
+	dentry = securityfs_create_file("policy", POLICY_FILE_FLAGS,
 					    ima_dir, NULL,
 					    &ima_measure_policy_ops);
-	if (IS_ERR(ima_policy)) {
-		ret = PTR_ERR(ima_policy);
+	if (IS_ERR(dentry)) {
+		ret = PTR_ERR(dentry);
 		goto out;
 	}
 
 	return 0;
 out:
-	securityfs_remove(ima_policy);
-	securityfs_remove(violations);
-	securityfs_remove(runtime_measurements_count);
-	securityfs_remove(ascii_runtime_measurements);
-	securityfs_remove(binary_runtime_measurements);
 	remove_securityfs_measurement_lists(ascii_securityfs_measurement_lists);
 	remove_securityfs_measurement_lists(binary_securityfs_measurement_lists);
 	securityfs_measurement_list_count = 0;
-- 
2.39.5

[PATCH 07/10] ima_fs: get rid of lookup-by-dentry stuff

[Al Viro]

lookup_template_data_hash_algo() machinery is used to locate the
matching ima_algo_array[] element at read time; securityfs
allows to stash that into inode->i_private at object creation
time, so there's no need to bother

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/ima/ima_fs.c | 82 +++++++--------------------------
 1 file changed, 16 insertions(+), 66 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 88421e8895c4..87045b09f120 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -116,28 +116,6 @@ void ima_putc(struct seq_file *m, void *data, int datalen)
 		seq_putc(m, *(char *)data++);
 }
 
-static struct dentry **ascii_securityfs_measurement_lists __ro_after_init;
-static struct dentry **binary_securityfs_measurement_lists __ro_after_init;
-static int securityfs_measurement_list_count __ro_after_init;
-
-static void lookup_template_data_hash_algo(int *algo_idx, enum hash_algo *algo,
-					   struct seq_file *m,
-					   struct dentry **lists)
-{
-	struct dentry *dentry;
-	int i;
-
-	dentry = file_dentry(m->file);
-
-	for (i = 0; i < securityfs_measurement_list_count; i++) {
-		if (dentry == lists[i]) {
-			*algo_idx = i;
-			*algo = ima_algo_array[i].algo;
-			break;
-		}
-	}
-}
-
 /* print format:
  *       32bit-le=pcr#
  *       char[n]=template digest
@@ -160,9 +138,10 @@ int ima_measurements_show(struct seq_file *m, void *v)
 	algo_idx = ima_sha1_idx;
 	algo = HASH_ALGO_SHA1;
 
-	if (m->file != NULL)
-		lookup_template_data_hash_algo(&algo_idx, &algo, m,
-					       binary_securityfs_measurement_lists);
+	if (m->file != NULL) {
+		algo_idx = (unsigned long)file_inode(m->file)->i_private;
+		algo = ima_algo_array[algo_idx].algo;
+	}
 
 	/* get entry */
 	e = qe->entry;
@@ -256,9 +235,10 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
 	algo_idx = ima_sha1_idx;
 	algo = HASH_ALGO_SHA1;
 
-	if (m->file != NULL)
-		lookup_template_data_hash_algo(&algo_idx, &algo, m,
-					       ascii_securityfs_measurement_lists);
+	if (m->file != NULL) {
+		algo_idx = (unsigned long)file_inode(m->file)->i_private;
+		algo = ima_algo_array[algo_idx].algo;
+	}
 
 	/* get entry */
 	e = qe->entry;
@@ -412,57 +392,33 @@ static const struct seq_operations ima_policy_seqops = {
 };
 #endif
 
-static void __init remove_securityfs_measurement_lists(struct dentry **lists)
-{
-	kfree(lists);
-}
-
 static int __init create_securityfs_measurement_lists(void)
 {
-	char file_name[NAME_MAX + 1];
-	struct dentry *dentry;
-	u16 algo;
-	int i;
-
-	securityfs_measurement_list_count = NR_BANKS(ima_tpm_chip);
+	int count = NR_BANKS(ima_tpm_chip);
 
 	if (ima_sha1_idx >= NR_BANKS(ima_tpm_chip))
-		securityfs_measurement_list_count++;
+		count++;
 
-	ascii_securityfs_measurement_lists =
-	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
-		    GFP_KERNEL);
-	if (!ascii_securityfs_measurement_lists)
-		return -ENOMEM;
-
-	binary_securityfs_measurement_lists =
-	    kcalloc(securityfs_measurement_list_count, sizeof(struct dentry *),
-		    GFP_KERNEL);
-	if (!binary_securityfs_measurement_lists)
-		return -ENOMEM;
-
-	for (i = 0; i < securityfs_measurement_list_count; i++) {
-		algo = ima_algo_array[i].algo;
+	for (int i = 0; i < count; i++) {
+		u16 algo = ima_algo_array[i].algo;
+		char file_name[NAME_MAX + 1];
+		struct dentry *dentry;
 
 		sprintf(file_name, "ascii_runtime_measurements_%s",
 			hash_algo_name[algo]);
 		dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
-						ima_dir, NULL,
+						ima_dir, (void *)(uintptr_t)i,
 						&ima_ascii_measurements_ops);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
 
-		ascii_securityfs_measurement_lists[i] = dentry;
-
 		sprintf(file_name, "binary_runtime_measurements_%s",
 			hash_algo_name[algo]);
 		dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
-						ima_dir, NULL,
+						ima_dir, (void *)(uintptr_t)i,
 						&ima_measurements_ops);
 		if (IS_ERR(dentry))
 			return PTR_ERR(dentry);
-
-		binary_securityfs_measurement_lists[i] = dentry;
 	}
 
 	return 0;
@@ -543,9 +499,6 @@ int __init ima_fs_init(void)
 	struct dentry *dentry;
 	int ret;
 
-	ascii_securityfs_measurement_lists = NULL;
-	binary_securityfs_measurement_lists = NULL;
-
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
 	if (IS_ERR(ima_dir))
 		return PTR_ERR(ima_dir);
@@ -600,9 +553,6 @@ int __init ima_fs_init(void)
 
 	return 0;
 out:
-	remove_securityfs_measurement_lists(ascii_securityfs_measurement_lists);
-	remove_securityfs_measurement_lists(binary_securityfs_measurement_lists);
-	securityfs_measurement_list_count = 0;
 	securityfs_remove(ima_symlink);
 	securityfs_remove(ima_dir);
 
-- 
2.39.5

[PATCH 08/10] evm_secfs: clear securityfs interactions

[Al Viro]

1) creation never returns NULL; error is reported as ERR_PTR()
2) no need to remove file before removing its parent

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/integrity/evm/evm_secfs.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index 9b907c2fee60..b0d2aad27850 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -17,7 +17,6 @@
 #include "evm.h"
 
 static struct dentry *evm_dir;
-static struct dentry *evm_init_tpm;
 static struct dentry *evm_symlink;
 
 #ifdef CONFIG_EVM_ADD_XATTRS
@@ -286,7 +285,7 @@ static int evm_init_xattrs(void)
 {
 	evm_xattrs = securityfs_create_file("evm_xattrs", 0660, evm_dir, NULL,
 					    &evm_xattr_ops);
-	if (!evm_xattrs || IS_ERR(evm_xattrs))
+	if (IS_ERR(evm_xattrs))
 		return -EFAULT;
 
 	return 0;
@@ -301,21 +300,22 @@ static int evm_init_xattrs(void)
 int __init evm_init_secfs(void)
 {
 	int error = 0;
+	struct dentry *dentry;
 
 	evm_dir = securityfs_create_dir("evm", integrity_dir);
-	if (!evm_dir || IS_ERR(evm_dir))
+	if (IS_ERR(evm_dir))
 		return -EFAULT;
 
-	evm_init_tpm = securityfs_create_file("evm", 0660,
-					      evm_dir, NULL, &evm_key_ops);
-	if (!evm_init_tpm || IS_ERR(evm_init_tpm)) {
+	dentry = securityfs_create_file("evm", 0660,
+				      evm_dir, NULL, &evm_key_ops);
+	if (IS_ERR(dentry)) {
 		error = -EFAULT;
 		goto out;
 	}
 
 	evm_symlink = securityfs_create_symlink("evm", NULL,
 						"integrity/evm/evm", NULL);
-	if (!evm_symlink || IS_ERR(evm_symlink)) {
+	if (IS_ERR(evm_symlink)) {
 		error = -EFAULT;
 		goto out;
 	}
@@ -328,7 +328,6 @@ int __init evm_init_secfs(void)
 	return 0;
 out:
 	securityfs_remove(evm_symlink);
-	securityfs_remove(evm_init_tpm);
 	securityfs_remove(evm_dir);
 	return error;
 }
-- 
2.39.5

[PATCH 09/10] ipe: don't bother with removal of files in directory we'll be removing

[Al Viro]

... and use securityfs_remove() instead of securityfs_recursive_remove()

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 security/ipe/fs.c        | 32 ++++++++++++--------------------
 security/ipe/policy_fs.c |  4 ++--
 2 files changed, 14 insertions(+), 22 deletions(-)

diff --git a/security/ipe/fs.c b/security/ipe/fs.c
index f40e50bfd2e7..0bb9468b8026 100644
--- a/security/ipe/fs.c
+++ b/security/ipe/fs.c
@@ -12,11 +12,8 @@
 #include "policy.h"
 #include "audit.h"
 
-static struct dentry *np __ro_after_init;
 static struct dentry *root __ro_after_init;
 struct dentry *policy_root __ro_after_init;
-static struct dentry *audit_node __ro_after_init;
-static struct dentry *enforce_node __ro_after_init;
 
 /**
  * setaudit() - Write handler for the securityfs node, "ipe/success_audit"
@@ -200,27 +197,26 @@ static int __init ipe_init_securityfs(void)
 {
 	int rc = 0;
 	struct ipe_policy *ap;
+	struct dentry *dentry;
 
 	if (!ipe_enabled)
 		return -EOPNOTSUPP;
 
 	root = securityfs_create_dir("ipe", NULL);
-	if (IS_ERR(root)) {
-		rc = PTR_ERR(root);
-		goto err;
-	}
+	if (IS_ERR(root))
+		return PTR_ERR(root);
 
-	audit_node = securityfs_create_file("success_audit", 0600, root,
+	dentry = securityfs_create_file("success_audit", 0600, root,
 					    NULL, &audit_fops);
-	if (IS_ERR(audit_node)) {
-		rc = PTR_ERR(audit_node);
+	if (IS_ERR(dentry)) {
+		rc = PTR_ERR(dentry);
 		goto err;
 	}
 
-	enforce_node = securityfs_create_file("enforce", 0600, root, NULL,
+	dentry = securityfs_create_file("enforce", 0600, root, NULL,
 					      &enforce_fops);
-	if (IS_ERR(enforce_node)) {
-		rc = PTR_ERR(enforce_node);
+	if (IS_ERR(dentry)) {
+		rc = PTR_ERR(dentry);
 		goto err;
 	}
 
@@ -237,18 +233,14 @@ static int __init ipe_init_securityfs(void)
 			goto err;
 	}
 
-	np = securityfs_create_file("new_policy", 0200, root, NULL, &np_fops);
-	if (IS_ERR(np)) {
-		rc = PTR_ERR(np);
+	dentry = securityfs_create_file("new_policy", 0200, root, NULL, &np_fops);
+	if (IS_ERR(dentry)) {
+		rc = PTR_ERR(dentry);
 		goto err;
 	}
 
 	return 0;
 err:
-	securityfs_remove(np);
-	securityfs_remove(policy_root);
-	securityfs_remove(enforce_node);
-	securityfs_remove(audit_node);
 	securityfs_remove(root);
 	return rc;
 }
diff --git a/security/ipe/policy_fs.c b/security/ipe/policy_fs.c
index db26032ccbe1..9d92d8a14b13 100644
--- a/security/ipe/policy_fs.c
+++ b/security/ipe/policy_fs.c
@@ -438,7 +438,7 @@ static const struct ipefs_file policy_subdir[] = {
  */
 void ipe_del_policyfs_node(struct ipe_policy *p)
 {
-	securityfs_recursive_remove(p->policyfs);
+	securityfs_remove(p->policyfs);
 	p->policyfs = NULL;
 }
 
@@ -485,6 +485,6 @@ int ipe_new_policyfs_node(struct ipe_policy *p)
 
 	return 0;
 err:
-	securityfs_recursive_remove(policyfs);
+	securityfs_remove(policyfs);
 	return rc;
 }
-- 
2.39.5

[PATCH 10/10] tpm: don't bother with removal of files in directory we'll be removing

[Al Viro]

FWIW, there is a reliable indication of removal - ->i_nlink going to 0 ;-)

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/char/tpm/eventlog/common.c | 46 ++++++++----------------------
 include/linux/tpm.h                |  2 +-
 2 files changed, 13 insertions(+), 35 deletions(-)

diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
index 4c0bbba64ee5..691813d2a5a2 100644
--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -32,7 +32,7 @@ static int tpm_bios_measurements_open(struct inode *inode,
 	struct tpm_chip *chip;
 
 	inode_lock(inode);
-	if (!inode->i_private) {
+	if (!inode->i_nlink) {
 		inode_unlock(inode);
 		return -ENODEV;
 	}
@@ -105,7 +105,7 @@ static int tpm_read_log(struct tpm_chip *chip)
 void tpm_bios_log_setup(struct tpm_chip *chip)
 {
 	const char *name = dev_name(&chip->dev);
-	unsigned int cnt;
+	struct dentry *dentry;
 	int log_version;
 	int rc = 0;
 
@@ -117,14 +117,12 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 		return;
 	log_version = rc;
 
-	cnt = 0;
-	chip->bios_dir[cnt] = securityfs_create_dir(name, NULL);
+	chip->bios_dir = securityfs_create_dir(name, NULL);
 	/* NOTE: securityfs_create_dir can return ENODEV if securityfs is
 	 * compiled out. The caller should ignore the ENODEV return code.
 	 */
-	if (IS_ERR(chip->bios_dir[cnt]))
-		goto err;
-	cnt++;
+	if (IS_ERR(chip->bios_dir))
+		return;
 
 	chip->bin_log_seqops.chip = chip;
 	if (log_version == EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
@@ -135,14 +133,13 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 			&tpm1_binary_b_measurements_seqops;
 
 
-	chip->bios_dir[cnt] =
+	dentry =
 	    securityfs_create_file("binary_bios_measurements",
-				   0440, chip->bios_dir[0],
+				   0440, chip->bios_dir,
 				   (void *)&chip->bin_log_seqops,
 				   &tpm_bios_measurements_ops);
-	if (IS_ERR(chip->bios_dir[cnt]))
+	if (IS_ERR(dentry))
 		goto err;
-	cnt++;
 
 	if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
 
@@ -150,42 +147,23 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 		chip->ascii_log_seqops.seqops =
 			&tpm1_ascii_b_measurements_seqops;
 
-		chip->bios_dir[cnt] =
+		dentry =
 			securityfs_create_file("ascii_bios_measurements",
-					       0440, chip->bios_dir[0],
+					       0440, chip->bios_dir,
 					       (void *)&chip->ascii_log_seqops,
 					       &tpm_bios_measurements_ops);
-		if (IS_ERR(chip->bios_dir[cnt]))
+		if (IS_ERR(dentry))
 			goto err;
-		cnt++;
 	}
 
 	return;
 
 err:
-	chip->bios_dir[cnt] = NULL;
 	tpm_bios_log_teardown(chip);
 	return;
 }
 
 void tpm_bios_log_teardown(struct tpm_chip *chip)
 {
-	int i;
-	struct inode *inode;
-
-	/* securityfs_remove currently doesn't take care of handling sync
-	 * between removal and opening of pseudo files. To handle this, a
-	 * workaround is added by making i_private = NULL here during removal
-	 * and to check it during open(), both within inode_lock()/unlock().
-	 * This design ensures that open() either safely gets kref or fails.
-	 */
-	for (i = (TPM_NUM_EVENT_LOG_FILES - 1); i >= 0; i--) {
-		if (chip->bios_dir[i]) {
-			inode = d_inode(chip->bios_dir[i]);
-			inode_lock(inode);
-			inode->i_private = NULL;
-			inode_unlock(inode);
-			securityfs_remove(chip->bios_dir[i]);
-		}
-	}
+	securityfs_remove(chip->bios_dir);
 }
diff --git a/include/linux/tpm.h b/include/linux/tpm.h
index a3d8305e88a5..9894c104dc93 100644
--- a/include/linux/tpm.h
+++ b/include/linux/tpm.h
@@ -182,7 +182,7 @@ struct tpm_chip {
 	unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */
 	bool duration_adjusted;
 
-	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
+	struct dentry *bios_dir;
 
 	const struct attribute_group *groups[3 + TPM_MAX_HASHES];
 	unsigned int groups_cnt;
-- 
2.39.5

Re: [PATCH 09/10] ipe: don't bother with removal of files in directory we'll be removing

[Fan Wu]

On Wed, Jun 11, 2025 at 8:12 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> ... and use securityfs_remove() instead of securityfs_recursive_remove()
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
>  security/ipe/fs.c        | 32 ++++++++++++--------------------
>  security/ipe/policy_fs.c |  4 ++--
>  2 files changed, 14 insertions(+), 22 deletions(-)
>

Acked-by: Fan Wu <wufan@kernel.org>

These changes look good to me. I ran our ipe test suite and it works well.

However, I didn't try fault injection to trigger the dentry creation
failure. I will try it later.

-Fan

Re: [PATCH 07/10] ima_fs: get rid of lookup-by-dentry stuff

[Mimi Zohar]

On Thu, 2025-06-12 at 04:11 +0100, Al Viro wrote:
> lookup_template_data_hash_algo() machinery is used to locate the
> matching ima_algo_array[] element at read time; securityfs
> allows to stash that into inode->i_private at object creation
> time, so there's no need to bother
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Really nice clean up.

thanks,

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

Re: [PATCH 06/10] ima_fs: don't bother with removal of files in directory we'll be removing

[Mimi Zohar]

On Thu, 2025-06-12 at 04:11 +0100, Al Viro wrote:
> removal of parent takes all children out
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

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

Re: [PATCH 08/10] evm_secfs: clear securityfs interactions

[Mimi Zohar]

On Thu, 2025-06-12 at 04:11 +0100, Al Viro wrote:
> 1) creation never returns NULL; error is reported as ERR_PTR()
> 2) no need to remove file before removing its parent
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Nice cleanup.

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

Re: [PATCH 09/10] ipe: don't bother with removal of files in directory we'll be removing

[Fan Wu]

On Thu, Jun 12, 2025 at 10:43 AM Fan Wu <wufan@kernel.org> wrote:
>
> On Wed, Jun 11, 2025 at 8:12 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > ... and use securityfs_remove() instead of securityfs_recursive_remove()
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> >  security/ipe/fs.c        | 32 ++++++++++++--------------------
> >  security/ipe/policy_fs.c |  4 ++--
> >  2 files changed, 14 insertions(+), 22 deletions(-)
> >
>
> Acked-by: Fan Wu <wufan@kernel.org>
>
> These changes look good to me. I ran our ipe test suite and it works well.
>
> However, I didn't try fault injection to trigger the dentry creation
> failure. I will try it later.
>

I tried tracing the reference count with and without this patch set. I
found that without the patch, there were indeed dentry leaks in the
ipe policy folder, and this patch set has successfully fixed them.

-Fan

Re: [PATCHES][CFR][CFT] securityfs cleanups and fixes

[Al Viro]

On Thu, Jun 12, 2025 at 04:09:51AM +0100, Al Viro wrote:

> Branch (6.16-rc1-based) lives in
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.securityfs
> Individual patches in followups.
> 
> Help with testing and review would be very welcome.

Seeing that no complaints have materialized, into -next it goes (with
Acked-by/Tested-by attached)...

Re: [PATCHES][CFR][CFT] securityfs cleanups and fixes

[Paul Moore]

On Tue, Jun 24, 2025 at 9:47 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jun 12, 2025 at 04:09:51AM +0100, Al Viro wrote:
>
> > Branch (6.16-rc1-based) lives in
> > git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #work.securityfs
> > Individual patches in followups.
> >
> > Help with testing and review would be very welcome.
>
> Seeing that no complaints have materialized, into -next it goes (with
> Acked-by/Tested-by attached)...

Thanks Al, I appreciate the help cleaning this up.

-- 
paul-moore.com